chore(mem2reg): add a few regression tests by asterite · Pull Request #9615 · noir-lang/noir

asterite · 2025-08-22T16:33:24Z

Description

Problem

Resolves #9398
Resolves #9415
Resolves #9455
Resolves #9439

Summary

It seems all of the issues were fixed by #9632 so this PR just adds regression tests.

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: `443250d`	Previous: `92aa75d`	Ratio
`sha512-100-bytes`	`0.124` s	`0.101` s	`1.23`

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

CC: @TomAFrench

vezenovm · 2025-08-22T16:56:17Z

(it's also interesting that this doesn't introduce any regressions in test programs...)

This isn't totally surprising as otherwise we would have been triggering this bug.

vezenovm · 2025-08-22T16:57:01Z

that method's name is maybe misleading

Agreed it is misleading

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

jfecher · 2025-08-22T17:23:39Z

I wonder if this was meant to track a reference being linked to a parent reference...

That is the case. Looking at the comment on the function:

/// Remember that `result` is the result of dereferencing `address`. This is important to
/// track aliasing when references are stored within other references.

It's to track that a reference result comes from dereferencing another reference. You can say its for references in general but since mem2reg only cares about values that contain references we don't care about results which don't contain references. So we don't care about loading from a &mut Field but we do want to remember loading from a &mut &mut Field.

asterite · 2025-08-26T16:50:40Z

@jfecher @vezenovm I added a small comment but I'm not sure it helps much.

I wonder if we could merge this PR as it is right now, mainly because it fixes 4 bugs and contains regression tests for them, making it easier to refactor to this code (like this refactor).

vezenovm · 2025-08-26T16:59:23Z

@jfecher @vezenovm I added a small comment but I'm not sure it helps much.

I wonder if we could merge this PR as it is right now, mainly because it fixes 4 bugs and contains regression tests for them, making it easier to refactor to this code (like this refactor).

I still would prefer we appropriately marked the result of two different ArrayGet's from the same array containing references as aliases as I do think this is the correct solution. I can take a closer look shortly to see if this fix can be done easily.

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: `8f798b7`	Previous: `c6835b5`	Ratio
`test_report_zkpassport_noir-ecdsa_`	`3` s	`1` s	`3`

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

CC: @TomAFrench

asterite · 2025-08-26T18:03:25Z

In the end all of this was already working in master, #9632 probably fixed all the issues. So this PR now just adds regression tests.

test_programs/execution_success/regression_9398/src/main.nr

Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: don't thread-bomb unnecessarily (noir-lang/noir#9643) chore(mem2reg): Only add to per function last_loads if load is not removed (noir-lang/noir#9647) chore(mem2reg): add a few regression tests (noir-lang/noir#9615) fix: Monomorphize function values as pairs of `(constrained, unconstrained)` (noir-lang/noir#9484) fix(mem2reg): Mark block parameters with unknown alias sets in presence of nested references (noir-lang/noir#9629) fix(ssa): Put some default in `Value::uninitialized` for references in the SSA interpreter (noir-lang/noir#9603) feat(ssa_fuzzer): ecdsa blackbox functions (noir-lang/noir#9584) fix(mem2reg): missing alias from block parameter to its argument (noir-lang/noir#9640) fix(acir_gen): A slice might be a nested Array, not a flattened DynamicArray (noir-lang/noir#9600) chore: add another mem2reg regression for #9613 (noir-lang/noir#9635) chore: document remove_if_else (in preparation for audit) (noir-lang/noir#9621) fix(mem2reg): Assume all function reference parameters have an unknown alias set with nested references (noir-lang/noir#9632) chore: add a regression test for #9613 (noir-lang/noir#9630) fix: Revert "feat(mem2reg): address last known value is independent of its… (noir-lang/noir#9628) fix(inlining): Do not inline globals and lower them during ACIR gen (noir-lang/noir#9626) chore(brillig): Include function name with `--count-array-copies` debug information (noir-lang/noir#9623) chore: use `assert_ssa_does_not_change` throughout all SSA tests (noir-lang/noir#9625) chore: only run remove_paired_rc in brillig functions (noir-lang/noir#9624) chore: some inlining refactors (noir-lang/noir#9622) chore(mem2reg): avoid redundant PostOrder computation (noir-lang/noir#9620) chore: bump external pinned commits (noir-lang/noir#9618) chore: document intrinsics (noir-lang/noir#9382) fix: Make inc/dec_rc impure (noir-lang/noir#9617) chore: greenlight `checked_to_unchecked` for audits (noir-lang/noir#9537) feat(mem2reg): address last known value is independent of its aliases (noir-lang/noir#9613) fix: Fix if-else alias in mem2reg (noir-lang/noir#9611) END_COMMIT_OVERRIDE

fix(mem2reg): consider ArrayGet to be a dereference

443250d

github-actions bot reviewed Aug 22, 2025

View reviewed changes

asterite requested a review from a team August 22, 2025 16:53

vezenovm reviewed Aug 22, 2025

View reviewed changes

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

asterite added 2 commits August 26, 2025 13:25

Merge branch 'master' into ab/mem2reg_array_get_dereference

46dccee

Try to explain

4fc34a4

asterite added 2 commits August 26, 2025 14:25

Merge branch 'master' into ab/mem2reg_array_get_dereference

7987c55

Apparently the issues got already fixed in a previous PR

8f798b7

github-actions bot reviewed Aug 26, 2025

View reviewed changes

asterite changed the title ~~fix(mem2reg): consider ArrayGet to be a dereference~~ chore(mem2reg): add a few regression tests Aug 26, 2025

Undo change

f1450bc

vezenovm approved these changes Aug 26, 2025

View reviewed changes

test_programs/execution_success/regression_9398/src/main.nr Show resolved Hide resolved

asterite added 2 commits August 26, 2025 16:40

Merge branch 'master' into ab/mem2reg_array_get_dereference

562f60a

Move test

7ea29df

asterite enabled auto-merge August 26, 2025 19:46

asterite added this pull request to the merge queue Aug 26, 2025

Merged via the queue into master with commit ec24082 Aug 26, 2025
122 checks passed

asterite deleted the ab/mem2reg_array_get_dereference branch August 26, 2025 20:46

AztecBot mentioned this pull request Aug 27, 2025

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(mem2reg): add a few regression tests#9615

chore(mem2reg): add a few regression tests#9615
asterite merged 8 commits intomasterfrom
ab/mem2reg_array_get_dereference

asterite commented Aug 22, 2025 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

vezenovm commented Aug 22, 2025

Uh oh!

vezenovm commented Aug 22, 2025

Uh oh!

Uh oh!

jfecher commented Aug 22, 2025

Uh oh!

asterite commented Aug 26, 2025

Uh oh!

vezenovm commented Aug 26, 2025

Uh oh!

github-actions bot left a comment

Uh oh!

asterite commented Aug 26, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

asterite commented Aug 22, 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

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

vezenovm commented Aug 22, 2025

Uh oh!

vezenovm commented Aug 22, 2025

Uh oh!

Uh oh!

jfecher commented Aug 22, 2025

Uh oh!

asterite commented Aug 26, 2025

Uh oh!

vezenovm commented Aug 26, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

asterite commented Aug 26, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asterite commented Aug 22, 2025 •

edited

Loading