fix(mem2reg): Keep last stores used in MakeArray by vezenovm · Pull Request #8743 · noir-lang/noir

vezenovm · 2025-05-30T21:22:01Z

Description

Problem*

Resolves #8739
Resolves #8755
Resolves #8761

Summary*

We have an optimization that runs through our remaining last stores and checks whether they are used anywhere. If not, we remove those last stores. This optimization was not accounting for references used in arrays.

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.

… stores

github-actions

ACVM Benchmarks

Details

Benchmark suite	Current: `3bfa172`	Previous: `b21a44c`	Ratio
`purely_sequential_opcodes`	`258367` ns/iter (`± 526`)	`253587` ns/iter (`± 2804`)	`1.02`
`perfectly_parallel_opcodes`	`234912` ns/iter (`± 4074`)	`224811` ns/iter (`± 1860`)	`1.04`
`perfectly_parallel_batch_inversion_opcodes`	`3571661` ns/iter (`± 915`)	`3215175` ns/iter (`± 33797`)	`1.11`

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

github-actions

Execution Time

Details

Benchmark suite	Current: `3bfa172`	Previous: `b21a44c`	Ratio
`private-kernel-inner`	`0.028` s	`0.028` s	`1`
`private-kernel-reset`	`0.163` s	`0.163` s	`1`
`private-kernel-tail`	`0.011` s	`0.012` s	`0.92`
`rollup-base-private`	`0.3` s	`0.303` s	`0.99`
`rollup-base-public`	`0.195` s	`0.196` s	`0.99`
`rollup-block-root`	`11.1` s	`11.1` s	`1`
`rollup-merge`	`0.004` s	`0.004` s	`1`
`rollup-root`	`0.009` s	`0.009` s	`1`
`semaphore-depth-10`	`0.02` s	`0.02` s	`1`

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

github-actions

Artifact Size

Details

Benchmark suite	Current: `3bfa172`	Previous: `b21a44c`	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

Compilation Time

Details

Benchmark suite	Current: `3bfa172`	Previous: `b21a44c`	Ratio
`private-kernel-inner`	`2.374` s	`2.308` s	`1.03`
`private-kernel-reset`	`7.862` s	`7.758` s	`1.01`
`private-kernel-tail`	`1.052` s	`1.112` s	`0.95`
`rollup-base-private`	`15.98` s	`16.2` s	`0.99`
`rollup-base-public`	`14.24` s	`13.76` s	`1.03`
`rollup-block-root-empty`	`1.256` s	`1.318` s	`0.95`
`rollup-block-root-single-tx`	`121` s	`130` s	`0.93`
`rollup-block-root`	`122` s	`130` s	`0.94`
`rollup-merge`	`1.124` s	`1.044` s	`1.08`
`rollup-root`	`1.512` s	`1.518` s	`1.00`
`semaphore-depth-10`	`0.813` s	`0.805` s	`1.01`

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

github-actions

Test Suite Duration

Details

Benchmark suite	Current: `3bfa172`	Previous: `b21a44c`	Ratio
`test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr`	`68` s	`68` s	`1`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts`	`109` s	`124` s	`0.88`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob`	`42` s	`49` s	`0.86`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib`	`209` s	`221` s	`0.95`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib`	`270` s	`254` s	`1.06`
`test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types`	`69` s	`69` s	`1`
`test_report_noir-lang_noir-bignum_`	`389` s	`385` s	`1.01`
`test_report_noir-lang_noir_bigcurve_`	`250` s	`238` s	`1.05`
`test_report_noir-lang_sha512_`	`31` s	`30` s	`1.03`
`test_report_zkpassport_noir_rsa_`	`2` s	`2` s	`1`

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

github-actions

Compilation Memory

Details

Benchmark suite	Current: `3bfa172`	Previous: `b21a44c`	Ratio
`private-kernel-inner`	`292.95` MB	`293` MB	`1.00`
`private-kernel-reset`	`533.91` MB	`533.91` MB	`1`
`private-kernel-tail`	`192.81` MB	`192.82` MB	`1.00`
`rollup-base-private`	`1380` MB	`1380` MB	`1`
`rollup-base-public`	`1540` MB	`1540` MB	`1`
`rollup-block-root-empty`	`342.95` MB	`342.95` MB	`1`
`rollup-block-root-single-tx`	`7830` MB	`7830` MB	`1`
`rollup-block-root`	`7830` MB	`7830` MB	`1`
`rollup-merge`	`327.16` MB	`327.17` MB	`1.00`
`rollup-root`	`381.47` MB	`381.44` 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: `3bfa172`	Previous: `b21a44c`	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/mem2reg.rs

test_programs/execution_success/regression_8739/src/main.nr

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

compiler/noirc_evaluator/src/ssa/opt/mem2reg.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: `eb851ee`	Previous: `b636856`	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

…mv/keep-ref-in-make-array

vezenovm · 2025-06-03T18:22:38Z

Added a regression test for #8755 that looks to still be broken under a minimum aggressive inliner, so will need to update that that.

vezenovm · 2025-06-04T14:16:50Z

This is ready for review again.

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

asterite

Looks good!

jfecher

LGTM

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>

vezenovm added 2 commits May 30, 2025 17:20

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

76077f6

… stores

cleanup

39865fa

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

github-actions bot reviewed May 30, 2025

View reviewed changes

vezenovm mentioned this pull request May 30, 2025

fix(ssa): Experiment with removing the last stores optimization in mem2reg #8745

Closed

5 tasks

github-actions bot reviewed May 30, 2025

View reviewed changes

vezenovm added 3 commits June 2, 2025 15:12

snapshots

7047fb2

snaps

a18835f

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

c604fd7

vezenovm marked this pull request as ready for review June 2, 2025 16:16

vezenovm requested a review from a team June 2, 2025 16:16

jfecher reviewed Jun 2, 2025

View reviewed changes

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

test_programs/execution_success/regression_8739/src/main.nr Outdated Show resolved Hide resolved

vezenovm and others added 2 commits June 2, 2025 12:31

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

db225ed

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

update naming and test snaps

1ae27d8

vezenovm requested a review from jfecher June 2, 2025 16:35

jfecher reviewed Jun 2, 2025

View reviewed changes

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

github-actions bot reviewed Jun 2, 2025

View reviewed changes

combine sets

0ae34e6

vezenovm requested a review from jfecher June 2, 2025 17:29

vezenovm added 6 commits June 3, 2025 15:49

add 8755 regression and mem2reg unit test

892ae97

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

153c19d

add regression 8761

5002cae

Merge remote-tracking branch 'origin/mv/keep-ref-in-make-array' into …

c81a04a

…mv/keep-ref-in-make-array

snaps

bf2153b

nargo fmt

eb851ee

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

d3c008f

aakoshh mentioned this pull request Jun 4, 2025

Execution crash: condition value is not a boolean: MismatchedBitSize #8786

Closed

fmt and cleanup

3bfa172

vezenovm mentioned this pull request Jun 5, 2025

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

Merged

5 tasks

asterite reviewed Jun 6, 2025

View reviewed changes

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

asterite approved these changes Jun 6, 2025

View reviewed changes

jfecher approved these changes Jun 6, 2025

View reviewed changes

vezenovm enabled auto-merge June 6, 2025 13:40

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

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

vezenovm deleted the mv/keep-ref-in-make-array branch June 6, 2025 14:15

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

AztecBot mentioned this pull request Jun 10, 2025

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

Closed

Conversation

vezenovm commented May 30, 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 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

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

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!

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 3, 2025

Uh oh!

vezenovm commented Jun 4, 2025

Uh oh!

Uh oh!

asterite left a comment

Choose a reason for hiding this comment

Uh oh!

jfecher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vezenovm commented May 30, 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

github-actions bot left a comment •

edited

Loading