-
Notifications
You must be signed in to change notification settings - Fork 326
feat: keep last loads from predecessors in mem2reg #9492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACVM Benchmarks
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opcode count
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compilation Time
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Execution Time
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Artifact Size
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compilation Memory
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Execution Memory
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test Suite Duration
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
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 fmt
on default settings.