fix(ssa): Do not fail for static assertions in a map over empty dynamic slices by vezenovm · Pull Request #9060 · noir-lang/noir

vezenovm · 2025-06-30T15:39:13Z

Description

Problem*

Resolves #7710

Summary*

This change ended up being more involved than expected:

If we are in an empty loop (e.g. the upper bound is not greater than the lower bound), we skip evaluation a static assertion and do not insert it back into the SSA. For now I chose to still error out if the static assertion is inside of a dynamic loop. However, this may not be sufficient on its own, perhaps we should add a warning to not place static assertions under dynamic loops.
With non-aggressive inlining we can have loop bounds from function parameters that would otherwise resolve to constants if the function was inlined. For example, the regression test added in this PR would fail with the minimum inliner aggressiveness and only the change from (1). I chose to simply always inline functions which contains calls to static assertions.
We can still have nested functions calls where we pass along a constant call argument through many calls. To fully handle (2) I switched inlining to operate with a feedback loop until a fixed point. This type of logic is common in modern compilers and is something we already do in multiple SSA passes.

Additional Context

A full fix to handling constants call arguments would be #8321. However, this is quite an involved change so I decided (2) and (3) above provided a nice way to iteratively fix this bug and propagate constants for static assertions.

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib`	`655` s	`541` s	`1.21`
`test_report_noir-lang_noir_bigcurve_`	`324` s	`251` s	`1.29`
`test_report_zkpassport_noir_rsa_`	`2` s	`1` s	`2`

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

github-actions

ACVM Benchmarks

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`purely_sequential_opcodes`	`249558` ns/iter (`± 1050`)	`257657` ns/iter (`± 2210`)	`0.97`
`perfectly_parallel_opcodes`	`220060` ns/iter (`± 3196`)	`228310` ns/iter (`± 3038`)	`0.96`
`perfectly_parallel_batch_inversion_opcodes`	`2807352` ns/iter (`± 2800`)	`2787393` ns/iter (`± 2850`)	`1.01`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions

Execution Time

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`private-kernel-inner`	`0.026` s	`0.026` s	`1`
`private-kernel-reset`	`0.161` s	`0.162` s	`0.99`
`private-kernel-tail`	`0.011` s	`0.011` s	`1`
`rollup-base-private`	`0.291` s	`0.291` s	`1`
`rollup-base-public`	`0.185` s	`0.183` s	`1.01`
`rollup-block-root`	`13.3` s	`13.2` s	`1.01`
`rollup-merge`	`0.002` s	`0.002` s	`1`
`rollup-root`	`0.004` s	`0.004` s	`1`
`semaphore-depth-10`	`0.026` s	`0.02` s	`1.30`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions

Compilation Time

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`private-kernel-inner`	`2.52` s	`2.362` s	`1.07`
`private-kernel-reset`	`8.204` s	`7.944` s	`1.03`
`private-kernel-tail`	`1.144` s	`1.184` s	`0.97`
`rollup-base-private`	`16.92` s	`15.5` s	`1.09`
`rollup-base-public`	`15.2` s	`13.46` s	`1.13`
`rollup-block-root-empty`	`20.58` s	`19.14` s	`1.08`
`rollup-block-root-single-tx`	`200` s	`189` s	`1.06`
`rollup-block-root`	`195` s	`184` s	`1.06`
`rollup-merge`	`1.31` s	`1.292` s	`1.01`
`rollup-root`	`1.39` s	`1.442` s	`0.96`
`semaphore-depth-10`	`0.796` s	`0.765` s	`1.04`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`semaphore-depth-10`	`0.026` s	`0.02` s	`1.30`

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

github-actions

Artifact Size

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`private-kernel-inner`	`1127.4` KB	`1127.4` KB	`1`
`private-kernel-reset`	`2067.9` KB	`2067.9` KB	`1`
`private-kernel-tail`	`585` KB	`585` KB	`1`
`rollup-base-private`	`4934.4` KB	`4934.4` KB	`1`
`rollup-base-public`	`3975.6` KB	`3975.6` KB	`1`
`rollup-block-root-empty`	`3871.1` KB	`3868.7` KB	`1.00`
`rollup-block-root-single-tx`	`32733.4` KB	`32731.7` KB	`1.00`
`rollup-block-root`	`32752.7` KB	`32749.7` KB	`1.00`
`rollup-merge`	`176.8` KB	`176.8` KB	`1`
`rollup-root`	`389.5` KB	`389.5` KB	`1`
`semaphore-depth-10`	`636.4` KB	`636.4` KB	`1`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions

Test Suite Duration

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr`	`100` s	`99` s	`1.01`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts`	`147` s	`148` s	`0.99`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob`	`167` s	`177` s	`0.94`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib`	`218` s	`219` s	`1.00`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib`	`655` s	`541` s	`1.21`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types`	`91` s	`93` s	`0.98`
`test_report_noir-lang_noir-bignum_`	`379` s	`376` s	`1.01`
`test_report_noir-lang_noir_bigcurve_`	`324` s	`251` s	`1.29`
`test_report_noir-lang_sha512_`	`16` s	`14` s	`1.14`
`test_report_zkpassport_noir-ecdsa_`	`104` s	`98` s	`1.06`
`test_report_zkpassport_noir_rsa_`	`2` s	`1` s	`2`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions

Execution Memory

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`private-kernel-inner`	`202.71` MB	`202.71` MB	`1`
`private-kernel-reset`	`226.5` MB	`226.5` MB	`1`
`private-kernel-tail`	`178.04` MB	`178.04` MB	`1`
`rollup-base-private`	`508.01` MB	`508.01` MB	`1`
`rollup-base-public`	`441.22` MB	`441.22` MB	`1`
`rollup-block-root`	`1510` MB	`1510` MB	`1`
`rollup-merge`	`330.09` MB	`330.09` MB	`1`
`rollup-root`	`332.7` MB	`332.7` MB	`1`
`semaphore_depth_10`	`71.01` MB	`71.01` MB	`1`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions

Compilation Memory

Details

Benchmark suite	Current: `56b199e`	Previous: `d12ebe7`	Ratio
`private-kernel-inner`	`280.13` MB	`280.13` MB	`1`
`private-kernel-reset`	`534.01` MB	`534.01` MB	`1`
`private-kernel-tail`	`184.58` MB	`184.58` MB	`1`
`rollup-base-private`	`1400` MB	`1400` MB	`1`
`rollup-base-public`	`1530` MB	`1530` MB	`1`
`rollup-block-root-empty`	`1090` MB	`1090` MB	`1`
`rollup-block-root-single-tx`	`9380` MB	`9380` MB	`1`
`rollup-block-root`	`9390` MB	`9390` MB	`1`
`rollup-merge`	`333.56` MB	`333.56` MB	`1`
`rollup-root`	`343.59` MB	`343.59` MB	`1`
`semaphore_depth_10`	`106.39` MB	`106.39` MB	`1`

This comment was automatically generated by workflow using github-action-benchmark.

compiler/noirc_evaluator/src/ssa/opt/inlining.rs

TomAFrench · 2025-07-01T08:44:58Z

merging master to run benchmarks again

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `2b7b601`	Previous: `d692001`	Ratio
`rollup-block-root`	`262` s	`182` s	`1.44`

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

vezenovm · 2025-07-01T10:29:43Z

merging master to run benchmarks again

Running once more without the re-inlining gate #9060 (comment) after #9069

… mv/static-assert-empty-map

TomAFrench · 2025-07-02T14:01:15Z

@vezenovm Looks like there's still issues with the merge.

TomAFrench

Looks good, there's a few nits plus a bad merge.

compiler/noirc_evaluator/src/ssa/opt/inlining/inline_info.rs

compiler/noirc_evaluator/src/ssa/opt/assert_constant.rs

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

… mv/static-assert-empty-map

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore: remove redundant associated constant lookup (noir-lang/noir#9114) chore: test that associated function and constant with the same name collide (noir-lang/noir#9112) feat: Allow TraitAsType syntax to refer to associated constants in expressions (noir-lang/noir#9041) chore: clippy (noir-lang/noir#9101) chore(fuzz): Recursively generate lvalue for multi dimensional arrays and nested tuples (noir-lang/noir#9086) fix!: remove `hash_to_field` from stdlib (noir-lang/noir#9098) chore(docs): Brillig gen (noir-lang/noir#9085) chore(fuzz): Test AST print-and-parse roundtrip (noir-lang/noir#9083) fix(ssa): Do not fail for static assertions in a map over empty dynamic slices (noir-lang/noir#9060) chore: merge `RangeCheckFailed` and `RangeCheckFailedWithMessage` (noir-lang/noir#9093) chore(fuzz): Capture printed output in `comptime_vs_brillig_direct` (noir-lang/noir#9090) chore(debug): Add trait constraint to string helper (noir-lang/noir#9082) chore: bump some deps (noir-lang/noir#9076) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

vezenovm added 4 commits June 30, 2025 11:39

static assertions in a map over an emtpy array

59c1f82

remove unnecessary clone

9fb570c

always inline functions containing calls to static assertions

b4e9fbc

cleanup

92f99a2

vezenovm changed the title ~~fix(ssa): Do not fail for static assertions in a map over empty arrays~~ fix(ssa): Do not fail for static assertions in a map over empty dynamic slices Jun 30, 2025

github-actions bot reviewed Jun 30, 2025

View reviewed changes

vezenovm added the bench-show Display benchmark results on PR label Jun 30, 2025

Merge branch 'master' into mv/static-assert-empty-map

f15069f

github-actions bot reviewed Jun 30, 2025

View reviewed changes

gate inline loop only for static assertions

b226e76

vezenovm marked this pull request as ready for review June 30, 2025 18:15

vezenovm requested a review from a team June 30, 2025 18:15

vezenovm commented Jun 30, 2025

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/inlining.rs Outdated Show resolved Hide resolved

vezenovm mentioned this pull request Jun 30, 2025

chore: Switch to rustc-hash #9064

Closed

5 tasks

Merge branch 'master' into mv/static-assert-empty-map

2b7b601

github-actions bot reviewed Jul 1, 2025

View reviewed changes

vezenovm added 2 commits July 1, 2025 10:28

try inlining once more without static assertions gate and new benches

66fe71c

Merge branch 'master' into mv/static-assert-empty-map

3db15bc

vezenovm added 3 commits July 1, 2025 11:03

snaps

4eaec09

Merge remote-tracking branch 'origin/mv/static-assert-empty-map' into…

0a8904d

… mv/static-assert-empty-map

Merge branch 'master' into mv/static-assert-empty-map

97a51db

merge conflicts w/ master

8acd07d

TomAFrench approved these changes Jul 2, 2025

View reviewed changes

vezenovm and others added 4 commits July 2, 2025 10:29

Update compiler/noirc_evaluator/src/ssa/opt/assert_constant.rs

e4fa042

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

handle runtimeerorr now returned from inlining

f8bf726

simplify test and pr comments

bdace94

Merge branch 'master' into mv/static-assert-empty-map

635476f

vezenovm enabled auto-merge July 2, 2025 15:32

vezenovm added 2 commits July 2, 2025 15:56

bump timeout for rollup-block-root-single-tx

352dc4c

Merge remote-tracking branch 'origin/mv/static-assert-empty-map' into…

56b199e

… mv/static-assert-empty-map

vezenovm added this pull request to the merge queue Jul 2, 2025

Merged via the queue into master with commit 0098c45 Jul 2, 2025
118 checks passed

vezenovm deleted the mv/static-assert-empty-map branch July 2, 2025 16:43

noirwhal mentioned this pull request Jul 2, 2025

chore: Release Noir(1.0.0-beta.8) #8892

Merged

AztecBot mentioned this pull request Jul 3, 2025

chore: Bump Noir reference AztecProtocol/aztec-packages#15489

Merged

Conversation

vezenovm commented Jun 30, 2025

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ACVM Benchmarks

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Execution Time

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Compilation Time

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Artifact Size

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Test Suite Duration

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Execution Memory

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Compilation Memory

Uh oh!

Uh oh!

TomAFrench commented Jul 1, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

vezenovm commented Jul 1, 2025

Uh oh!

TomAFrench commented Jul 2, 2025

Uh oh!

TomAFrench left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading