feat: keep last loads from predecessors in mem2reg#9492
Conversation
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
|
Closing because I just wanted to be sure this could be a potential optimization before capturing an audit issue. See #9493 |
There was a problem hiding this comment.
⚠️ 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: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
test_report_noir-lang_noir-bignum_ |
519 s |
383 s |
1.36 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
2c37b1c to
de154df
Compare
Yeah if the only regression is with a max inliner setting I think this is a good optimization. It is unlikely projects in production will want a maximally aggressive inliner. |
I added bench-show as mem2reg is one of the more memory heavy passes so I want to see the effect on our benchmarks. bigcurve looks to be the only library that caused an alert, but it would be good to see all the results. |
There was a problem hiding this comment.
ACVM Benchmarks
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
purely_sequential_opcodes |
252098 ns/iter (± 773) |
250253 ns/iter (± 364) |
1.01 |
perfectly_parallel_opcodes |
222284 ns/iter (± 4653) |
217849 ns/iter (± 1429) |
1.02 |
perfectly_parallel_batch_inversion_opcodes |
2793154 ns/iter (± 2048) |
2786444 ns/iter (± 1165) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Opcode count
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
14792 opcodes |
14792 opcodes |
1 |
private-kernel-reset |
68868 opcodes |
68868 opcodes |
1 |
private-kernel-tail |
11177 opcodes |
11177 opcodes |
1 |
rollup-base-private |
221335 opcodes |
221323 opcodes |
1.00 |
rollup-base-public |
159954 opcodes |
159942 opcodes |
1.00 |
rollup-block-root-empty |
68106 opcodes |
68106 opcodes |
1 |
rollup-block-root-single-tx |
963855 opcodes |
963851 opcodes |
1.00 |
rollup-block-root |
965141 opcodes |
965137 opcodes |
1.00 |
rollup-merge |
1409 opcodes |
1409 opcodes |
1 |
rollup-root |
2631 opcodes |
2631 opcodes |
1 |
semaphore-depth-10 |
5700 opcodes |
5700 opcodes |
1 |
sha512-100-bytes |
13173 opcodes |
13173 opcodes |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Time
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
1.68 s |
1.638 s |
1.03 |
private-kernel-reset |
7.884 s |
7.916 s |
1.00 |
private-kernel-tail |
1.37 s |
1.334 s |
1.03 |
rollup-base-private |
15.36 s |
15.44 s |
0.99 |
rollup-base-public |
13.26 s |
13.5 s |
0.98 |
rollup-block-root-empty |
22.88 s |
21.18 s |
1.08 |
rollup-block-root-single-tx |
209 s |
194 s |
1.08 |
rollup-block-root |
192 s |
195 s |
0.98 |
rollup-merge |
1.36 s |
1.356 s |
1.00 |
rollup-root |
1.45 s |
1.496 s |
0.97 |
semaphore-depth-10 |
0.749 s |
0.773 s |
0.97 |
sha512-100-bytes |
1.599 s |
1.663 s |
0.96 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Time
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.017 s |
0.014 s |
1.21 |
private-kernel-reset |
0.154 s |
0.154 s |
1 |
private-kernel-tail |
0.01 s |
0.011 s |
0.91 |
rollup-base-private |
0.264 s |
0.266 s |
0.99 |
rollup-base-public |
0.159 s |
0.162 s |
0.98 |
rollup-block-root |
13.4 s |
13.2 s |
1.02 |
rollup-merge |
0.002 s |
0.002 s |
1 |
rollup-root |
0.004 s |
0.004 s |
1 |
semaphore-depth-10 |
0.018 s |
0.019 s |
0.95 |
sha512-100-bytes |
0.102 s |
0.089 s |
1.15 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
⚠️ 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: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.017 s |
0.014 s |
1.21 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
Artifact Size
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
708.9 KB |
709.1 KB |
1.00 |
private-kernel-reset |
2032.5 KB |
2033.1 KB |
1.00 |
private-kernel-tail |
535.2 KB |
535.9 KB |
1.00 |
rollup-base-private |
4319.4 KB |
4317.7 KB |
1.00 |
rollup-base-public |
3329.9 KB |
3329.6 KB |
1.00 |
rollup-block-root-empty |
3847 KB |
3846.8 KB |
1.00 |
rollup-block-root-single-tx |
30729.7 KB |
30728.8 KB |
1.00 |
rollup-block-root |
30774.2 KB |
30774.4 KB |
1.00 |
rollup-merge |
187 KB |
187 KB |
1 |
rollup-root |
388.9 KB |
388.9 KB |
1 |
semaphore-depth-10 |
631.5 KB |
631.5 KB |
1 |
sha512-100-bytes |
525.2 KB |
525.2 KB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Memory
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
239.7 MB |
239.91 MB |
1.00 |
private-kernel-reset |
549.1 MB |
549.11 MB |
1.00 |
private-kernel-tail |
213.84 MB |
213.85 MB |
1.00 |
rollup-base-private |
1350 MB |
1350 MB |
1 |
rollup-base-public |
1400 MB |
1420 MB |
0.99 |
rollup-block-root-empty |
1090 MB |
1090 MB |
1 |
rollup-block-root-single-tx |
9550 MB |
9550 MB |
1 |
rollup-block-root |
9560 MB |
9560 MB |
1 |
rollup-merge |
330.56 MB |
330.56 MB |
1 |
rollup-root |
341.38 MB |
341.38 MB |
1 |
semaphore_depth_10 |
104.76 MB |
104.76 MB |
1 |
sha512_100_bytes |
234.74 MB |
234.83 MB |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Memory
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
private-kernel-inner |
206.72 MB |
206.72 MB |
1 |
private-kernel-reset |
243.53 MB |
243.54 MB |
1.00 |
private-kernel-tail |
195.94 MB |
195.95 MB |
1.00 |
rollup-base-private |
500.25 MB |
500.25 MB |
1 |
rollup-base-public |
432.89 MB |
432.89 MB |
1 |
rollup-block-root |
1500 MB |
1500 MB |
1 |
rollup-merge |
328.19 MB |
328.19 MB |
1 |
rollup-root |
330.55 MB |
330.55 MB |
1 |
semaphore_depth_10 |
69.5 MB |
69.5 MB |
1 |
sha512_100_bytes |
54.99 MB |
54.99 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Test Suite Duration
Details
| Benchmark suite | Current: fb8014a | Previous: 8e8ce66 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr |
100 s |
97 s |
1.03 |
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 |
152 s |
161 s |
0.94 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
210 s |
220 s |
0.95 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib |
33 s |
34 s |
0.97 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
659 s |
642 s |
1.03 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
103 s |
101 s |
1.02 |
test_report_noir-lang_noir-bignum_ |
519 s |
383 s |
1.36 |
test_report_noir-lang_noir_bigcurve_ |
358 s |
336 s |
1.07 |
test_report_noir-lang_sha256_ |
15 s |
15 s |
1 |
test_report_noir-lang_sha512_ |
14 s |
14 s |
1 |
test_report_zkpassport_noir-ecdsa_ |
1 s |
3 s |
0.33 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
The test suite timings seem to jump around quite a bit, but the other compilation time and memory benchmarks all look good. |
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore: some mem2reg refactors regarding expressions and aliases (noir-lang/noir#9610) feat: keep last loads from predecessors in mem2reg (noir-lang/noir#9492) chore: Update flattening docs (noir-lang/noir#9588) chore: remove redundant globals creation (noir-lang/noir#9606) chore: simplify Expression in mem2reg (noir-lang/noir#9599) chore: remove duplicate `contains_reference` in mem2reg (noir-lang/noir#9602) chore!: remove `verify_signature_slice` methods from stdlib (noir-lang/noir#9597) fix(expand): correctly handle nested dereferences (noir-lang/noir#9598) fix(ssa): Do not simplify on lhs being zero for shifts (noir-lang/noir#9596) chore: store last loads in `HashSet` instead of `HashMap` in mem2reg (noir-lang/noir#9498) chore: `--no-ssa-locations` for `nargo interpret` and show negative values when printing SSA (noir-lang/noir#9586) fix: `assert_constant` refactors and fixes from audit (noir-lang/noir#9547) fix(ssa): Consider `shl` and `shr` to have side effects (noir-lang/noir#9580) fix: avoid invalid cast in `remove_bit_shifts` (noir-lang/noir#9570) fix(mem2reg): Consider aliases of a loaded address to be loaded from as well (noir-lang/noir#9567) fix: Consume correct number of fields when printing references (noir-lang/noir#9579) chore: Add a section for numeric type aliases (noir-lang/noir#9589) chore(remove_paired_rc): Add various unit tests (noir-lang/noir#9425) fix: incorrect max bit size in `remove_bit_shifts` (noir-lang/noir#9585) chore(ssa): Simplify shl/shr identity operations (noir-lang/noir#9587) chore: greenlight `brillig_array_get_and_set` for audits (noir-lang/noir#9540) chore(ssa): Update comments on `loop_invariant` for audit and some missing unit tests (noir-lang/noir#9574) chore: Switch to node v22.15.0 (noir-lang/noir#9582) chore: Update unrolling docs for audit (noir-lang/noir#9572) chore: greenlight `array_set_optimization` (noir-lang/noir#9560) fix(acir_gen): Keep range checks before side effects (noir-lang/noir#9340) END_COMMIT_OVERRIDE
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore: some mem2reg refactors regarding expressions and aliases (noir-lang/noir#9610) feat: keep last loads from predecessors in mem2reg (noir-lang/noir#9492) chore: Update flattening docs (noir-lang/noir#9588) chore: remove redundant globals creation (noir-lang/noir#9606) chore: simplify Expression in mem2reg (noir-lang/noir#9599) chore: remove duplicate `contains_reference` in mem2reg (noir-lang/noir#9602) chore!: remove `verify_signature_slice` methods from stdlib (noir-lang/noir#9597) fix(expand): correctly handle nested dereferences (noir-lang/noir#9598) fix(ssa): Do not simplify on lhs being zero for shifts (noir-lang/noir#9596) chore: store last loads in `HashSet` instead of `HashMap` in mem2reg (noir-lang/noir#9498) chore: `--no-ssa-locations` for `nargo interpret` and show negative values when printing SSA (noir-lang/noir#9586) fix: `assert_constant` refactors and fixes from audit (noir-lang/noir#9547) fix(ssa): Consider `shl` and `shr` to have side effects (noir-lang/noir#9580) fix: avoid invalid cast in `remove_bit_shifts` (noir-lang/noir#9570) fix(mem2reg): Consider aliases of a loaded address to be loaded from as well (noir-lang/noir#9567) fix: Consume correct number of fields when printing references (noir-lang/noir#9579) chore: Add a section for numeric type aliases (noir-lang/noir#9589) chore(remove_paired_rc): Add various unit tests (noir-lang/noir#9425) fix: incorrect max bit size in `remove_bit_shifts` (noir-lang/noir#9585) chore(ssa): Simplify shl/shr identity operations (noir-lang/noir#9587) chore: greenlight `brillig_array_get_and_set` for audits (noir-lang/noir#9540) chore(ssa): Update comments on `loop_invariant` for audit and some missing unit tests (noir-lang/noir#9574) chore: Switch to node v22.15.0 (noir-lang/noir#9582) chore: Update unrolling docs for audit (noir-lang/noir#9572) chore: greenlight `array_set_optimization` (noir-lang/noir#9560) fix(acir_gen): Keep range checks before side effects (noir-lang/noir#9340) END_COMMIT_OVERRIDE
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore: some mem2reg refactors regarding expressions and aliases (noir-lang/noir#9610) feat: keep last loads from predecessors in mem2reg (noir-lang/noir#9492) chore: Update flattening docs (noir-lang/noir#9588) chore: remove redundant globals creation (noir-lang/noir#9606) chore: simplify Expression in mem2reg (noir-lang/noir#9599) chore: remove duplicate `contains_reference` in mem2reg (noir-lang/noir#9602) chore!: remove `verify_signature_slice` methods from stdlib (noir-lang/noir#9597) fix(expand): correctly handle nested dereferences (noir-lang/noir#9598) fix(ssa): Do not simplify on lhs being zero for shifts (noir-lang/noir#9596) chore: store last loads in `HashSet` instead of `HashMap` in mem2reg (noir-lang/noir#9498) chore: `--no-ssa-locations` for `nargo interpret` and show negative values when printing SSA (noir-lang/noir#9586) fix: `assert_constant` refactors and fixes from audit (noir-lang/noir#9547) fix(ssa): Consider `shl` and `shr` to have side effects (noir-lang/noir#9580) fix: avoid invalid cast in `remove_bit_shifts` (noir-lang/noir#9570) fix(mem2reg): Consider aliases of a loaded address to be loaded from as well (noir-lang/noir#9567) fix: Consume correct number of fields when printing references (noir-lang/noir#9579) chore: Add a section for numeric type aliases (noir-lang/noir#9589) chore(remove_paired_rc): Add various unit tests (noir-lang/noir#9425) fix: incorrect max bit size in `remove_bit_shifts` (noir-lang/noir#9585) chore(ssa): Simplify shl/shr identity operations (noir-lang/noir#9587) chore: greenlight `brillig_array_get_and_set` for audits (noir-lang/noir#9540) chore(ssa): Update comments on `loop_invariant` for audit and some missing unit tests (noir-lang/noir#9574) chore: Switch to node v22.15.0 (noir-lang/noir#9582) chore: Update unrolling docs for audit (noir-lang/noir#9572) chore: greenlight `array_set_optimization` (noir-lang/noir#9560) fix(acir_gen): Keep range checks before side effects (noir-lang/noir#9340) END_COMMIT_OVERRIDE
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore: some mem2reg refactors regarding expressions and aliases (noir-lang/noir#9610) feat: keep last loads from predecessors in mem2reg (noir-lang/noir#9492) chore: Update flattening docs (noir-lang/noir#9588) chore: remove redundant globals creation (noir-lang/noir#9606) chore: simplify Expression in mem2reg (noir-lang/noir#9599) chore: remove duplicate `contains_reference` in mem2reg (noir-lang/noir#9602) chore!: remove `verify_signature_slice` methods from stdlib (noir-lang/noir#9597) fix(expand): correctly handle nested dereferences (noir-lang/noir#9598) fix(ssa): Do not simplify on lhs being zero for shifts (noir-lang/noir#9596) chore: store last loads in `HashSet` instead of `HashMap` in mem2reg (noir-lang/noir#9498) chore: `--no-ssa-locations` for `nargo interpret` and show negative values when printing SSA (noir-lang/noir#9586) fix: `assert_constant` refactors and fixes from audit (noir-lang/noir#9547) fix(ssa): Consider `shl` and `shr` to have side effects (noir-lang/noir#9580) fix: avoid invalid cast in `remove_bit_shifts` (noir-lang/noir#9570) fix(mem2reg): Consider aliases of a loaded address to be loaded from as well (noir-lang/noir#9567) fix: Consume correct number of fields when printing references (noir-lang/noir#9579) chore: Add a section for numeric type aliases (noir-lang/noir#9589) chore(remove_paired_rc): Add various unit tests (noir-lang/noir#9425) fix: incorrect max bit size in `remove_bit_shifts` (noir-lang/noir#9585) chore(ssa): Simplify shl/shr identity operations (noir-lang/noir#9587) chore: greenlight `brillig_array_get_and_set` for audits (noir-lang/noir#9540) chore(ssa): Update comments on `loop_invariant` for audit and some missing unit tests (noir-lang/noir#9574) chore: Switch to node v22.15.0 (noir-lang/noir#9582) chore: Update unrolling docs for audit (noir-lang/noir#9572) chore: greenlight `array_set_optimization` (noir-lang/noir#9560) fix(acir_gen): Keep range checks before side effects (noir-lang/noir#9340) END_COMMIT_OVERRIDE
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE chore: some mem2reg refactors regarding expressions and aliases (noir-lang/noir#9610) feat: keep last loads from predecessors in mem2reg (noir-lang/noir#9492) chore: Update flattening docs (noir-lang/noir#9588) chore: remove redundant globals creation (noir-lang/noir#9606) chore: simplify Expression in mem2reg (noir-lang/noir#9599) chore: remove duplicate `contains_reference` in mem2reg (noir-lang/noir#9602) chore!: remove `verify_signature_slice` methods from stdlib (noir-lang/noir#9597) fix(expand): correctly handle nested dereferences (noir-lang/noir#9598) fix(ssa): Do not simplify on lhs being zero for shifts (noir-lang/noir#9596) chore: store last loads in `HashSet` instead of `HashMap` in mem2reg (noir-lang/noir#9498) chore: `--no-ssa-locations` for `nargo interpret` and show negative values when printing SSA (noir-lang/noir#9586) fix: `assert_constant` refactors and fixes from audit (noir-lang/noir#9547) fix(ssa): Consider `shl` and `shr` to have side effects (noir-lang/noir#9580) fix: avoid invalid cast in `remove_bit_shifts` (noir-lang/noir#9570) fix(mem2reg): Consider aliases of a loaded address to be loaded from as well (noir-lang/noir#9567) fix: Consume correct number of fields when printing references (noir-lang/noir#9579) chore: Add a section for numeric type aliases (noir-lang/noir#9589) chore(remove_paired_rc): Add various unit tests (noir-lang/noir#9425) fix: incorrect max bit size in `remove_bit_shifts` (noir-lang/noir#9585) chore(ssa): Simplify shl/shr identity operations (noir-lang/noir#9587) chore: greenlight `brillig_array_get_and_set` for audits (noir-lang/noir#9540) chore(ssa): Update comments on `loop_invariant` for audit and some missing unit tests (noir-lang/noir#9574) chore: Switch to node v22.15.0 (noir-lang/noir#9582) chore: Update unrolling docs for audit (noir-lang/noir#9572) chore: greenlight `array_set_optimization` (noir-lang/noir#9560) fix(acir_gen): Keep range checks before side effects (noir-lang/noir#9340) END_COMMIT_OVERRIDE
Description
Problem
Resolves #9493
Summary
There's one regression among all of the performance improvements but it happens only with inliner=max. Definitely something to improve but maybe the improvements are worth it.
Additional Context
Documentation
Check one:
PR Checklist
cargo fmton default settings.