fix(licm): Preserve semantic ordering of side-effectual instructions when hoisting by vezenovm · Pull Request #8724 · noir-lang/noir

vezenovm · 2025-05-29T20:33:28Z

Description

Problem*

Resolves #8715
Resolves #8619

For code like this where x = 2 and y = 3:

fn main(x: u32, y: u32) {
  for i in 0..4 {
      if x == 2 {
        let y = i + x;
        assert_eq(y, 12);
      }
      assert_eq(x, 12);
  }
}

On master, this will fail at assert_eq(x, 12). However, we should fail at assert_eq(y, 12). This breaks the semantic correctness of the program. This can lead to much larger issues if we have oracles that we expect to be executed that are skipped by hoisting a failing constrain inappropriately.

Summary*

I added a new flag current_block_impure to the LoopInvariantContext. It tracks whether a non-hoisted loop instruction in the current block has side effects. If it does have side effects, we mark this block impure. This means any later control dependent instructions (e.g. predicate or side effectual instructions) cannot be hoisted.

current_block_impure is initially set per block when checking the current block's control dependence. We check whether any predecessor blocks of the current block have a side effectual instructions. We could pre-compute these similar to how we do for post dominance frontiers, but as this is an important correctness fix I felt I would rather get the PR up first and it looks like the compilation benchmarks have not degraded at all with this change.

Additional Context

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.

…h side effects will be executed before it

github-actions

ACVM Benchmarks

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`purely_sequential_opcodes`	`255423` ns/iter (`± 651`)	`254697` ns/iter (`± 2972`)	`1.00`
`perfectly_parallel_opcodes`	`225039` ns/iter (`± 3361`)	`223885` ns/iter (`± 1867`)	`1.01`
`perfectly_parallel_batch_inversion_opcodes`	`3210841` ns/iter (`± 2492`)	`3579518` ns/iter (`± 2207`)	`0.90`

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

github-actions · 2025-05-29T20:37:58Z

Changes to Brillig bytecode sizes

Generated at commit: c8d27d1e46fa652241d3ce54bd22c2a7e0c4a753, compared to commit: 13e2bd91b27f604ecaf7a666ccc4bea2ebef6358

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
poseidonsponge_x5_254_inliner_max	+2 ❌	+0.05%
regression_5252_inliner_max	+2 ❌	+0.05%

Full diff report 👇

Program	Brillig opcodes (+/-)	%
poseidonsponge_x5_254_inliner_max	3,682 (+2)	+0.05%
regression_5252_inliner_max	3,968 (+2)	+0.05%
poseidon_bn254_hash_width_3_inliner_zero	4,445 (+2)	+0.05%
poseidon_bn254_hash_width_3_inliner_max	4,858 (+2)	+0.04%

github-actions · 2025-05-29T20:38:11Z

Changes to number of Brillig opcodes executed

Generated at commit: c8d27d1e46fa652241d3ce54bd22c2a7e0c4a753, compared to commit: 13e2bd91b27f604ecaf7a666ccc4bea2ebef6358

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_5252_inliner_zero	+22,430 ❌	+2.48%
poseidonsponge_x5_254_inliner_zero	+4,486 ❌	+2.47%
regression_5252_inliner_min	+22,430 ❌	+2.45%

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_5252_inliner_zero	927,651 (+22,430)	+2.48%
poseidonsponge_x5_254_inliner_zero	186,325 (+4,486)	+2.47%
regression_5252_inliner_min	939,593 (+22,430)	+2.45%
poseidonsponge_x5_254_inliner_min	188,228 (+4,486)	+2.44%
poseidon_bn254_hash_width_3_inliner_min	171,132 (+3,030)	+1.80%
regression_5252_inliner_max	760,165 (+10,800)	+1.44%
poseidonsponge_x5_254_inliner_max	152,929 (+2,160)	+1.43%
poseidon_bn254_hash_width_3_inliner_max	137,371 (+1,509)	+1.11%
poseidon_bn254_hash_width_3_inliner_zero	167,783 (+1,509)	+0.91%
array_to_slice_inliner_max	950 (+3)	+0.32%
hashmap_inliner_max	51,510 (+12)	+0.02%
hashmap_inliner_zero	66,795 (+12)	+0.02%
hashmap_inliner_min	73,352 (+12)	+0.02%

github-actions

Compilation Time

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`private-kernel-inner`	`2.37` s	`2.354` s	`1.01`
`private-kernel-reset`	`7.526` s	`8.062` s	`0.93`
`private-kernel-tail`	`1.04` s	`1.214` s	`0.86`
`rollup-base-private`	`15.9` s	`16.34` s	`0.97`
`rollup-base-public`	`13.18` s	`13.8` s	`0.96`
`rollup-block-root-empty`	`1.248` s	`1.272` s	`0.98`
`rollup-block-root-single-tx`	`156` s	`128` s	`1.22`
`rollup-block-root`	`127` s	`130` s	`0.98`
`rollup-merge`	`1.076` s	`1.046` s	`1.03`
`rollup-root`	`1.51` s	`1.552` s	`0.97`
`semaphore-depth-10`	`0.819` s	`0.841` s	`0.97`

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

github-actions

Execution Time

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`private-kernel-inner`	`0.028` s	`0.028` s	`1`
`private-kernel-reset`	`0.162` s	`0.163` s	`0.99`
`private-kernel-tail`	`0.011` s	`0.012` s	`0.92`
`rollup-base-private`	`0.303` s	`0.302` s	`1.00`
`rollup-base-public`	`0.198` s	`0.192` s	`1.03`
`rollup-block-root`	`11.8` s	`11.5` s	`1.03`
`rollup-merge`	`0.004` s	`0.004` s	`1`
`rollup-root`	`0.009` s	`0.009` s	`1`
`semaphore-depth-10`	`0.019` s	`0.021` s	`0.90`

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

github-actions

Artifact Size

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`private-kernel-inner`	`1133` KB	`1133` KB	`1`
`private-kernel-reset`	`2064.4` KB	`2064.4` KB	`1`
`private-kernel-tail`	`588.8` KB	`588.8` KB	`1`
`rollup-base-private`	`4955.9` KB	`4955.9` KB	`1`
`rollup-base-public`	`3995.3` KB	`3995.3` KB	`1`
`rollup-block-root-empty`	`256.7` KB	`256.7` KB	`1`
`rollup-block-root-single-tx`	`25707.5` KB	`25707.5` KB	`1`
`rollup-block-root`	`25714.4` KB	`25714.4` KB	`1`
`rollup-merge`	`183.4` KB	`183.4` KB	`1`
`rollup-root`	`417.9` KB	`417.9` KB	`1`
`semaphore-depth-10`	`636.3` KB	`636.3` KB	`1`

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

github-actions

Test Suite Duration

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr`	`69` s	`70` s	`0.99`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts`	`110` s	`109` s	`1.01`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob`	`42` s	`45` s	`0.93`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib`	`203` s	`202` s	`1.00`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib`	`253` s	`247` s	`1.02`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types`	`71` s	`71` s	`1`
`test_report_noir-lang_noir-bignum_`	`410` s	`387` s	`1.06`
`test_report_noir-lang_noir_bigcurve_`	`256` s	`234` s	`1.09`
`test_report_noir-lang_sha512_`	`32` s	`30` s	`1.07`
`test_report_zkpassport_noir_rsa_`	`2` s	`3` s	`0.67`

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

github-actions

Compilation Memory

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`private-kernel-inner`	`293` MB	`292.94` MB	`1.00`
`private-kernel-reset`	`533.91` MB	`533.91` MB	`1`
`private-kernel-tail`	`192.81` MB	`192.8` MB	`1.00`
`rollup-base-private`	`1380` MB	`1380` MB	`1`
`rollup-base-public`	`1540` MB	`1540` MB	`1`
`rollup-block-root-empty`	`342.94` MB	`342.96` MB	`1.00`
`rollup-block-root-single-tx`	`7830` MB	`7150` MB	`1.10`
`rollup-block-root`	`7830` MB	`7160` MB	`1.09`
`rollup-merge`	`327.16` MB	`327.17` MB	`1.00`
`rollup-root`	`381.44` MB	`381.45` MB	`1.00`
`semaphore_depth_10`	`106.4` MB	`106.4` MB	`1`

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

github-actions

Execution Memory

Details

Benchmark suite	Current: `5a393cb`	Previous: `13e2bd9`	Ratio
`private-kernel-inner`	`202.02` MB	`202.02` MB	`1`
`private-kernel-reset`	`225.75` MB	`225.75` MB	`1`
`private-kernel-tail`	`177.16` MB	`177.16` MB	`1`
`rollup-base-private`	`489.94` MB	`489.94` MB	`1`
`rollup-base-public`	`423.45` MB	`423.45` MB	`1`
`rollup-block-root`	`1400` MB	`1400` MB	`1`
`rollup-merge`	`312.01` MB	`312.01` MB	`1`
`rollup-root`	`317.69` MB	`317.69` MB	`1`
`semaphore_depth_10`	`70.96` MB	`70.96` MB	`1`

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

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

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: `7acd0c2`	Previous: `5984575`	Ratio
`test_report_zkpassport_noir_rsa_`	`3` s	`2` s	`1.50`

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

CC: @TomAFrench

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

asterite

Looks good!

(though some tests are failing, maybe snapshots)

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: `5a393cb`	Previous: `13e2bd9`	Ratio
`rollup-block-root-single-tx`	`156` s	`128` s	`1.22`

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

CC: @TomAFrench

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: type unification tests, and try moving constants to the other side (noir-lang/noir#8807) fix!: disallow casting numeric to bool (noir-lang/noir#8703) fix: delay associated constants resolution (noir-lang/noir#8744) fix: right shift overflow to 0 (noir-lang/noir#8772) fix: use value predicated by range checks (noir-lang/noir#8778) fix: unify infix expressions by isolating unbound type variables (noir-lang/noir#8796) feat: use `asm` feature flag in arkworks (noir-lang/noir#8792) chore: add a failing test for #8780 (noir-lang/noir#8785) chore: Adjust the frequency of 'for' statements in ACIR fuzz generation (noir-lang/noir#8788) fix: Merge `replacement_type` and `is_function_type` in `defunctionalization` (noir-lang/noir#8784) chore(fuzz): Remove unreachable functions in the AST fuzzer (noir-lang/noir#8782) chore(docs): Copy attribute docs into versioned docs (noir-lang/noir#8777) fix(licm): Preserve semantic ordering of side-effectual instructions when hoisting (noir-lang/noir#8724) fix: Create SSA interpreter arguments from scratch for each invocation (noir-lang/noir#8762) chore: mark sha512 as non-critical (noir-lang/noir#8776) fix!: disallow specifying associated items via generics (noir-lang/noir#8756) fix: stop inserting instructions after break and continue (noir-lang/noir#8712) fix: Fix comptime casts of negative integer to field (noir-lang/noir#8696) chore(SSA): restrict `shr` and `shl` right-hand side to u8 (noir-lang/noir#8753) chore: bump some JS packages (noir-lang/noir#8771) chore: document `allow(dead_code)` and reorganize attributes (noir-lang/noir#8766) fix: Add missing cases for finding function values in `find_functions_as_values` (noir-lang/noir#8738) fix: correct bitsize in signed division (noir-lang/noir#8733) chore: remove noir-lang/noir_rsa from external libraries (noir-lang/noir#8752) chore: bump external pinned commits (noir-lang/noir#8747) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: type unification tests, and try moving constants to the other side (noir-lang/noir#8807) fix!: disallow casting numeric to bool (noir-lang/noir#8703) fix: delay associated constants resolution (noir-lang/noir#8744) fix: right shift overflow to 0 (noir-lang/noir#8772) fix: use value predicated by range checks (noir-lang/noir#8778) fix: unify infix expressions by isolating unbound type variables (noir-lang/noir#8796) feat: use `asm` feature flag in arkworks (noir-lang/noir#8792) chore: add a failing test for AztecProtocol#8780 (noir-lang/noir#8785) chore: Adjust the frequency of 'for' statements in ACIR fuzz generation (noir-lang/noir#8788) fix: Merge `replacement_type` and `is_function_type` in `defunctionalization` (noir-lang/noir#8784) chore(fuzz): Remove unreachable functions in the AST fuzzer (noir-lang/noir#8782) chore(docs): Copy attribute docs into versioned docs (noir-lang/noir#8777) fix(licm): Preserve semantic ordering of side-effectual instructions when hoisting (noir-lang/noir#8724) fix: Create SSA interpreter arguments from scratch for each invocation (noir-lang/noir#8762) chore: mark sha512 as non-critical (noir-lang/noir#8776) fix!: disallow specifying associated items via generics (noir-lang/noir#8756) fix: stop inserting instructions after break and continue (noir-lang/noir#8712) fix: Fix comptime casts of negative integer to field (noir-lang/noir#8696) chore(SSA): restrict `shr` and `shl` right-hand side to u8 (noir-lang/noir#8753) chore: bump some JS packages (noir-lang/noir#8771) chore: document `allow(dead_code)` and reorganize attributes (noir-lang/noir#8766) fix: Add missing cases for finding function values in `find_functions_as_values` (noir-lang/noir#8738) fix: correct bitsize in signed division (noir-lang/noir#8733) chore: remove noir-lang/noir_rsa from external libraries (noir-lang/noir#8752) chore: bump external pinned commits (noir-lang/noir#8747) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

vezenovm added 2 commits May 29, 2025 19:36

do not hoist control dependent instruction if another instruction wit…

7b98a71

…h side effects will be executed before it

check predecessor blocks and tests

82976b6

vezenovm added the bench-show Display benchmark results on PR label May 29, 2025

github-actions bot reviewed May 29, 2025

View reviewed changes

vezenovm added 2 commits May 29, 2025 20:43

cleanup

aedfd3a

Merge branch 'master' into mv/check-side-effects-licm

d1c5904

github-actions bot reviewed May 29, 2025

View reviewed changes

vezenovm requested a review from a team May 29, 2025 20:52

vezenovm marked this pull request as ready for review May 29, 2025 20:52

vezenovm mentioned this pull request May 30, 2025

LICM: Hoisted overflowing operation out of if <call func> { <overflowing> } where condition is always false #8619

Closed

asterite reviewed May 30, 2025

View reviewed changes

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

move has_side_effects onto Instruction

2645ba7

github-actions bot reviewed May 30, 2025

View reviewed changes

vezenovm commented Jun 2, 2025

View reviewed changes

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

vezenovm added 2 commits June 2, 2025 11:14

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

50aefe4

Merge branch 'master' into mv/check-side-effects-licm

84db4bf

vezenovm requested a review from asterite June 2, 2025 16:28

asterite reviewed Jun 2, 2025

View reviewed changes

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

asterite reviewed Jun 2, 2025

View reviewed changes

compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs Show resolved Hide resolved

add sanity hcecks that we do not override an impure block

6f8f27f

vezenovm requested a review from asterite June 2, 2025 19:24

asterite approved these changes Jun 2, 2025

View reviewed changes

snapshosts and do simplify constrain under more circumstances

d131b70

vezenovm added 5 commits June 2, 2025 23:25

fix test src

1da50f2

missing can_simplify_control_dependent_instruction

9e50747

snaps

7acd0c2

Merge branch 'master' into mv/check-side-effects-licm

55c3541

snaps again

5a393cb

vezenovm enabled auto-merge June 3, 2025 14:55

github-actions bot reviewed Jun 3, 2025

View reviewed changes

vezenovm added this pull request to the merge queue Jun 3, 2025

Merged via the queue into master with commit 1523ea1 Jun 3, 2025
121 checks passed

vezenovm deleted the mv/check-side-effects-licm branch June 3, 2025 15:33

noirwhal mentioned this pull request Jun 3, 2025

chore: Release Noir(1.0.0-beta.7) #8520

Merged

AztecBot mentioned this pull request Jun 4, 2025

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

Merged

AztecBot mentioned this pull request Jun 6, 2025

chore(next): release 0.88.0 AztecProtocol/aztec-packages#14495

Closed

Conversation

vezenovm commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

ACVM Benchmarks

Uh oh!

github-actions bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to Brillig bytecode sizes

🧾 Summary (10% most significant diffs)

Uh oh!

github-actions bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to number of Brillig opcodes executed

🧾 Summary (10% most significant diffs)

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

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

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

Compilation 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

Execution Memory

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

asterite left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vezenovm commented May 29, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot commented May 29, 2025 •

edited

Loading

github-actions bot commented May 29, 2025 •

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