fix: do not hoist control dependent cast by guipublic · Pull Request #8886 · noir-lang/noir

guipublic · 2025-06-11T17:14:21Z

Description

Problem*

Resolves #8841

Summary*

Cast instruction were marked as 'can be hoisted', even if they are control dependent, so loop invariant code motion were hoisting them out of their conditional statement.
However, range-check are properly marked as control-dependent, so when the cast is done after a range-check, and the cast is moved, the related range-check is not moved, making the cast not safe anymore.

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.

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

github-actions · 2025-06-11T17:20:39Z

Changes to number of Brillig opcodes executed

Generated at commit: c4f15fcd944687c538483277510110fa602e8519, compared to commit: 24c053fba747770cf8d3f813d22cfa003714dfb6

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
loop_invariant_regression_inliner_max	+12 ❌	+3.67%
loop_invariant_regression_inliner_zero	+12 ❌	+3.67%
higher_order_functions_inliner_zero	+23 ❌	+1.98%

Full diff report 👇

Program	Brillig opcodes (+/-)	%
loop_invariant_regression_inliner_max	339 (+12)	+3.67%
loop_invariant_regression_inliner_zero	339 (+12)	+3.67%
higher_order_functions_inliner_zero	1,186 (+23)	+1.98%
loop_invariant_regression_inliner_min	1,555 (+11)	+0.71%
regression_5252_inliner_zero	930,416 (+2,650)	+0.29%
poseidonsponge_x5_254_inliner_zero	186,877 (+530)	+0.28%
regression_5252_inliner_min	942,355 (+2,650)	+0.28%
poseidonsponge_x5_254_inliner_min	188,782 (+530)	+0.28%
poseidon_bn254_hash_width_3_inliner_min	171,524 (+392)	+0.23%
regression_5252_inliner_max	760,524 (+240)	+0.03%
poseidonsponge_x5_254_inliner_max	152,998 (+48)	+0.03%
poseidon_bn254_hash_width_3_inliner_max	137,407 (+36)	+0.03%
poseidon_bn254_hash_width_3_inliner_zero	167,819 (+36)	+0.02%

github-actions · 2025-06-11T17:21:06Z

Changes to circuit sizes

Generated at commit: c4f15fcd944687c538483277510110fa602e8519, compared to commit: 24c053fba747770cf8d3f813d22cfa003714dfb6

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
loop_invariant_regression	+37 ❌	+308.33%	+255 ❌	+9.20%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
loop_invariant_regression	49 (+37)	+308.33%	3,026 (+255)	+9.20%

github-actions · 2025-06-11T17:21:08Z

Changes to Brillig bytecode sizes

Generated at commit: c4f15fcd944687c538483277510110fa602e8519, compared to commit: 24c053fba747770cf8d3f813d22cfa003714dfb6

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
loop_invariant_regression_inliner_max	+72 ❌	+48.65%
loop_invariant_regression_inliner_zero	+72 ❌	+48.65%

Full diff report 👇

Program	Brillig opcodes (+/-)	%
loop_invariant_regression_inliner_max	220 (+72)	+48.65%
loop_invariant_regression_inliner_zero	220 (+72)	+48.65%
loop_invariant_regression_inliner_min	284 (+71)	+33.33%
higher_order_functions_inliner_zero	652 (+2)	+0.31%

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: `4f67560`	Previous: `24c053f`	Ratio
`test_report_zkpassport_noir-ecdsa_`	`117` s	`95` s	`1.23`
`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

⚠️ 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: `4f67560`	Previous: `24c053f`	Ratio
`rollup-merge`	`0.004` s	`0.003` s	`1.33`

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(LSP): suggest generic type methods (noir-lang/noir#8948) fix: Fix if/match tracking in last uses pass (noir-lang/noir#8935) fix(ssa): Swap Brillig index shift and DIE in minimal pipeline (noir-lang/noir#8946) fix: when macro parse error happens, discard warnings; also preserve unquoted token locations (noir-lang/noir#8944) chore: disable noncritical workflow steps which fail on external prs (noir-lang/noir#8939) chore(test): Add `--minimal-ssa` to integration tests (noir-lang/noir#8938) fix(noirc_evaluator): U128 Binary::And simplification (noir-lang/noir#8940) chore: Improve access to data required to extract Noir (noir-lang/noir#8793) chore: bump external pinned commits (noir-lang/noir#8920) chore(fuzz): Compare print output on failed programs (noir-lang/noir#8921) chore(fuzz): Display nightly fuzz status badge (noir-lang/noir#8932) fix(fuzz): Consider values returned from Brillig to ACIR as dynamic (noir-lang/noir#8931) fix(fuzz): Fix env var name in fuzzing workflow (noir-lang/noir#8929) chore(fuzz): Add nightly fuzz workflow with 5 minute budget (noir-lang/noir#8925) fix(mem2reg): Keep last store for a used nested array (noir-lang/noir#8917) chore(fuzz): Increase the number of deterministic test cases (noir-lang/noir#8872) fix: Create calls to `apply` before function values are changed to fields in defunctionalize (noir-lang/noir#8916) chore: Add various regression tests for mutable references (noir-lang/noir#8915) fix: handle return_data in the interpreter SSA CLI (noir-lang/noir#8914) feat(fuzz): Generate references in the AST fuzzer (noir-lang/noir#8728) fix(fuzz): Use an inline block to circumvent negation with overflow (noir-lang/noir#8911) fix: Inline global arrays with functions at their call site (noir-lang/noir#8905) chore: Casting ACIR vs Brillig regression tests (noir-lang/noir#8903) fix(mem2reg): Keep last store for reference in array used only in an array get (noir-lang/noir#8877) fix: more SSA interpreter fixes (noir-lang/noir#8904) chore: Ban dynamic array indices with reference elements (noir-lang/noir#8888) chore(ssa_fuzzer): Add SSA validation to the SSA fuzzer (noir-lang/noir#8901) chore(fuzz): More metamorphic rules (noir-lang/noir#8857) fix: catch unbound type variables during frontend compilation (noir-lang/noir#8686) fix: preserve functions which are used in `array_set` instructions (noir-lang/noir#8891) fix: assorted SSA interpreter fixes (noir-lang/noir#8893) fix(ssa): Signed cast simplification (noir-lang/noir#8862) feat: simplify apply function cfg immediately (noir-lang/noir#8895) fix: do not hoist control dependent cast (noir-lang/noir#8886) fix(licm): Account for negative bounds when checking whether a loop executes (noir-lang/noir#8889) chore: Release Noir(1.0.0-beta.7) (noir-lang/noir#8520) chore: test noir-ecdsa library in CI (noir-lang/noir#8859) chore: Incorrect clippy warning blocking PRs (noir-lang/noir#8861) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

cast are control dependent

fd8c4ff

vezenovm approved these changes Jun 11, 2025

View reviewed changes

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

github-actions bot reviewed Jun 11, 2025

View reviewed changes

guipublic and others added 2 commits June 12, 2025 09:28

code review: add a unit test

1b07e11

Merge branch 'master' into gd/issue_8841_bis

0a2dde2

TomAFrench enabled auto-merge June 12, 2025 09:36

guipublic and others added 2 commits June 12, 2025 12:37

snapshots

2aff4e6

Merge branch 'master' into gd/issue_8841_bis

a1ed20d

github-actions bot reviewed Jun 12, 2025

View reviewed changes

guipublic added 3 commits June 12, 2025 12:46

fix unit test

ae9bac7

snapshots

c05d7e2

fix unit test

4f67560

TomAFrench added this pull request to the merge queue Jun 12, 2025

Merged via the queue into master with commit 98d19fb Jun 12, 2025
118 checks passed

TomAFrench deleted the gd/issue_8841_bis branch June 12, 2025 13:58

noirwhal mentioned this pull request Jun 12, 2025

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

Merged

AztecBot mentioned this pull request Jun 13, 2025

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

Merged

guipublic mentioned this pull request Jun 13, 2025

ACIR != Brillig: Failed constraint, shouldn't run #8880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not hoist control dependent cast#8886

fix: do not hoist control dependent cast#8886
TomAFrench merged 8 commits intomasterfrom
gd/issue_8841_bis

guipublic commented Jun 11, 2025

Uh oh!

Uh oh!

github-actions bot commented Jun 11, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 11, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 11, 2025 •

edited

Loading

Uh oh!

github-actions bot left a comment •

edited

Loading

Uh oh!

github-actions bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

guipublic commented Jun 11, 2025

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

Uh oh!

Uh oh!

github-actions bot commented Jun 11, 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 commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to circuit sizes

🧾 Summary (10% most significant diffs)

Uh oh!

github-actions bot commented Jun 11, 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 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

⚠️ Performance Alert ⚠️

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jun 11, 2025 •

edited

Loading

github-actions bot commented Jun 11, 2025 •

edited

Loading

github-actions bot commented Jun 11, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading