fix(mem2reg): Keep last stores used in array returned from a function by vezenovm · Pull Request #8801 · noir-lang/noir

vezenovm · 2025-06-05T01:41:10Z

Description

Problem*

Resolves #8786

Builds upon #8743

Summary*

In #8743, we switched to mark all aliases of a Call's arguments unknown. In this PR if we mark all aliases a return termiantor's arguments unknown this also fixes the issue. This then keeps the store for a reference in an array that would otherwise have been removed. However, the crux of the bug is in fact that mark_all_unknown is not visiting any aliases for an array type that contains references #8801 (comment).

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.

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: `bc21dcc`	Previous: `2e5cd41`	Ratio
`rollup-merge`	`0.004` s	`0.003` s	`1.33`

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

CC: @TomAFrench

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: `9b79642`	Previous: `ce2fc37`	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

aakoshh

Great! I thought it's the return value not being marked, but also thought this must already be happening for simple references because that seems to work, and I assumed as long as we marked them in the call to make_array it should pick it up.

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

aakoshh · 2025-06-05T07:54:10Z

I think the fuzzer still finds failures after this; I'll try to isolate them in new tickets.

EDIT: It was #8803 which seems to be a different issue with mem2reg.

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

github-actions

⚠️ Performance Alert ⚠️

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

Benchmark suite	Current: `f00a126`	Previous: `ce2fc37`	Ratio
`perfectly_parallel_batch_inversion_opcodes`	`3575762` ns/iter (`± 1733`)	`2813355` ns/iter (`± 3956`)	`1.27`

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

CC: @TomAFrench

vezenovm · 2025-06-05T16:11:25Z

In the parent PR #8743 we were not appropriately labeling array aliases. To keep arrays containing references we updated the input to mark_all_unknown by collecting all aliases for every argument to a call or return. In fact, mark_all_unknown was just too restrictive. I have updated mark_all_unknown to access every alias from an array constant.

… stores

Co-authored-by: jfecher <jfecher11@gmail.com>

… to prevent bad last store opt

This reverts commit 5c451c9.

vezenovm · 2025-06-06T17:12:02Z

This is ready for review again. cc @jfecher

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore(docs): Update noirjs app page to use to beta.6 (noir-lang/noir#8853) fix: support recursive call to main function in SSA parser (noir-lang/noir#8760) chore(SSA): validate that constrain values have the same type (noir-lang/noir#8850) fix: Comptime field division should error when the rhs is zero (noir-lang/noir#8845) chore: redo typo PR by osrm (noir-lang/noir#8840) fix: (SSA interpreter) to_le_bits returns [u1; _], not [u8; _] (noir-lang/noir#8837) chore(ssa): Initial validation module (noir-lang/noir#8765) chore: bump external pinned commits (noir-lang/noir#8834) feat(fuzz): Generate arbitrary constraints (noir-lang/noir#8820) fix: bind self generic type in trait calls via a concrete type in more cases (noir-lang/noir#8827) fix(comptime): Overflow on shl (noir-lang/noir#8829) fix(interpreter): Return -1 for negative shr signed overflow or 0 for positive shr signed overflow (noir-lang/noir#8828) feat(ssa_fuzzer): branching + constrains (noir-lang/noir#8599) chore(docs): Add experimental warning in Debugger docs (noir-lang/noir#8824) fix: Thread errors through remove_if_else instead of panicing when the value merger finds reference values (noir-lang/noir#8783) fix(interpreter): Do not overflow on signed checked ops (noir-lang/noir#8806) feat: short circuit creation of `Type::InfixExpr` containing errors (noir-lang/noir#8826) fix(mem2reg): Keep last stores used in array returned from a function (noir-lang/noir#8801) chore(ci): `cargo clippy` CI script to save time (noir-lang/noir#8787) chore: only follow bindings on interface to `arithmetic` module (noir-lang/noir#8822) fix: bind self generic type in trait calls via a concrete type (noir-lang/noir#8825) chore(docs): Reorder tooling docs (noir-lang/noir#8742) chore: small fix for outdated docs (noir-lang/noir#8821) fix(mem2reg): Keep last stores used in MakeArray (noir-lang/noir#8743) fix!: Error when re-assigning a mutable reference (noir-lang/noir#8790) fix!: indexing arrays with non-u32 is now an error (noir-lang/noir#8804) fix: signed right shift overflows to 0 or -1 (noir-lang/noir#8805) chore(fuzz): Tool to minimize Noir programs with `cvise` (noir-lang/noir#8789) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

github-actions bot reviewed Jun 5, 2025

View reviewed changes

aakoshh approved these changes Jun 5, 2025

View reviewed changes

aakoshh reviewed Jun 5, 2025

View reviewed changes

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

aakoshh mentioned this pull request Jun 5, 2025

Execution crash: condition value is not a boolean: MismatchedBitSize (reassign mut &mut, no if-then) #8803

Closed

jfecher requested changes Jun 5, 2025

View reviewed changes

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

github-actions bot reviewed Jun 5, 2025

View reviewed changes

vezenovm requested a review from jfecher June 5, 2025 13:43

Base automatically changed from mv/keep-ref-in-make-array to master June 6, 2025 14:15

vezenovm and others added 18 commits June 6, 2025 14:47

keep track of references in makearray as to not remove them with last…

8abc80f

… stores

cleanup

20afa5f

Update test_programs/execution_success/regression_8739/src/main.nr

88fa5c7

Co-authored-by: jfecher <jfecher11@gmail.com>

update naming and test snaps

ed65867

combine sets

ca05b32

add regression 8761

fdbfc70

snaps

73a614a

nargo fmt

d731c31

fix last stores in makearray by setting a known empty alias set

c885ea3

fmt and cleanup

c1d84ad

mark all aliases unknown when returning

ccc3dda

fmt and snaps

fe080e4

add test

e569395

use aliasset helpers

be5d23b

try collecting all aliases without unifying the alias sets

946b2fb

clippy

b7c658c

fix mark_all_unknown

3a2eb88

revert collect_all_aliases changes

81cac48

vezenovm force-pushed the mv/keep-ref-in-returned-make-array branch from 774ee50 to 81cac48 Compare June 6, 2025 14:53

vezenovm changed the title ~~fix(ssa): Keep last stores used in array returned from a function~~ fix(mem2reg): Keep last stores used in array returned from a function Jun 6, 2025

vezenovm added 6 commits June 6, 2025 15:59

expose for_each_alias_of and insert into instruction_input_references…

8c22209

… to prevent bad last store opt

Merge branch 'master' into mv/keep-ref-in-returned-make-array

c772d06

clippy

331ed9f

documentation on binop macros

5c451c9

Revert "documentation on binop macros"

a4637ff

This reverts commit 5c451c9.

Merge branch 'master' into mv/keep-ref-in-returned-make-array

bc21dcc

jfecher approved these changes Jun 6, 2025

View reviewed changes

jfecher added this pull request to the merge queue Jun 6, 2025

Merged via the queue into master with commit ec955fb Jun 6, 2025
117 checks passed

jfecher deleted the mv/keep-ref-in-returned-make-array branch June 6, 2025 17:55

noirwhal mentioned this pull request Jun 6, 2025

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

Merged

AztecBot mentioned this pull request Jun 7, 2025

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

Merged

asterite mentioned this pull request Jun 9, 2025

Execution crash: value is not typed as brillig usize #8727

Closed

AztecBot mentioned this pull request Jun 10, 2025

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

Closed

vezenovm mentioned this pull request Jun 10, 2025

fix(mem2reg): Keep last store for reference in array used only in an array get #8877

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mem2reg): Keep last stores used in array returned from a function#8801

fix(mem2reg): Keep last stores used in array returned from a function#8801
jfecher merged 24 commits intomasterfrom
mv/keep-ref-in-returned-make-array

vezenovm commented Jun 5, 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!

aakoshh left a comment

Uh oh!

Uh oh!

aakoshh commented Jun 5, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot left a comment •

edited

Loading

Uh oh!

vezenovm commented Jun 5, 2025

Uh oh!

vezenovm commented Jun 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

vezenovm commented Jun 5, 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

⚠️ 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!

aakoshh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aakoshh commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

vezenovm commented Jun 5, 2025

Uh oh!

vezenovm commented Jun 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vezenovm commented Jun 5, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

aakoshh commented Jun 5, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading