-
Notifications
You must be signed in to change notification settings - Fork 327
fix(ssa): Constant fold Brillig calls using the SSA interpreter #9655
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
Conversation
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
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: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
test_report_noir-lang_noir_bigcurve_ |
347 s |
279 s |
1.24 |
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.
ACVM Benchmarks
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
purely_sequential_opcodes |
249850 ns/iter (± 674 ) |
248430 ns/iter (± 776 ) |
1.01 |
perfectly_parallel_opcodes |
218077 ns/iter (± 3815 ) |
218561 ns/iter (± 6896 ) |
1.00 |
perfectly_parallel_batch_inversion_opcodes |
2778732 ns/iter (± 2207 ) |
2782471 ns/iter (± 11623 ) |
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 Time
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
private-kernel-inner |
0.014 s |
0.019 s |
0.74 |
private-kernel-reset |
0.155 s |
0.153 s |
1.01 |
private-kernel-tail |
0.01 s |
0.01 s |
1 |
rollup-base-private |
0.267 s |
0.264 s |
1.01 |
rollup-base-public |
0.162 s |
0.164 s |
0.99 |
rollup-block-root |
12.9 s |
12.9 s |
1 |
rollup-merge |
0.002 s |
0.002 s |
1 |
rollup-root |
0.004 s |
0.004 s |
1 |
semaphore-depth-10 |
0.019 s |
0.019 s |
1 |
sha512-100-bytes |
0.099 s |
0.1 s |
0.99 |
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: 38979ee | Previous: cc0c20d | Ratio |
---|---|---|---|
private-kernel-inner |
0.019 s |
0.014 s |
1.36 |
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.
Compilation Time
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
private-kernel-inner |
1.78 s |
1.75 s |
1.02 |
private-kernel-reset |
7.796 s |
7.686 s |
1.01 |
private-kernel-tail |
1.33 s |
1.426 s |
0.93 |
rollup-base-private |
15.26 s |
15.36 s |
0.99 |
rollup-base-public |
14.3 s |
14.16 s |
1.01 |
rollup-block-root-empty |
22.98 s |
21.32 s |
1.08 |
rollup-block-root-single-tx |
203 s |
199 s |
1.02 |
rollup-block-root |
200 s |
210 s |
0.95 |
rollup-merge |
1.308 s |
1.358 s |
0.96 |
rollup-root |
1.46 s |
1.47 s |
0.99 |
semaphore-depth-10 |
0.787 s |
0.777 s |
1.01 |
sha512-100-bytes |
1.539 s |
1.624 s |
0.95 |
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: ad2a326 | Previous: b544e60 | 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 |
221335 opcodes |
1 |
rollup-base-public |
159954 opcodes |
159954 opcodes |
1 |
rollup-block-root-empty |
68106 opcodes |
68106 opcodes |
1 |
rollup-block-root-single-tx |
964509 opcodes |
964509 opcodes |
1 |
rollup-block-root |
965795 opcodes |
965795 opcodes |
1 |
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.
Artifact Size
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
private-kernel-inner |
708.9 KB |
708.9 KB |
1 |
private-kernel-reset |
2032.7 KB |
2032.7 KB |
1 |
private-kernel-tail |
536.4 KB |
536.5 KB |
1.00 |
rollup-base-private |
4319.4 KB |
4320.9 KB |
1.00 |
rollup-base-public |
3334.1 KB |
3334.3 KB |
1.00 |
rollup-block-root-empty |
3856.2 KB |
3857 KB |
1.00 |
rollup-block-root-single-tx |
30756.1 KB |
30756.4 KB |
1.00 |
rollup-block-root |
30785.7 KB |
30786.1 KB |
1.00 |
rollup-merge |
188.2 KB |
188.2 KB |
1 |
rollup-root |
390.4 KB |
390.5 KB |
1.00 |
semaphore-depth-10 |
631.5 KB |
631.5 KB |
1 |
sha512-100-bytes |
525.5 KB |
525.5 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.
Execution Memory
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
private-kernel-inner |
212.98 MB |
212.98 MB |
1 |
private-kernel-reset |
246.18 MB |
246.18 MB |
1 |
private-kernel-tail |
197.81 MB |
197.81 MB |
1 |
rollup-base-private |
501.57 MB |
501.57 MB |
1 |
rollup-base-public |
434.03 MB |
434.03 MB |
1 |
rollup-block-root |
1500 MB |
1500 MB |
1 |
rollup-merge |
328.36 MB |
328.36 MB |
1 |
rollup-root |
330.89 MB |
330.89 MB |
1 |
semaphore_depth_10 |
69.53 MB |
69.53 MB |
1 |
sha512_100_bytes |
55.03 MB |
55.03 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.
Compilation Memory
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
private-kernel-inner |
239.62 MB |
239.58 MB |
1.00 |
private-kernel-reset |
549.15 MB |
549.15 MB |
1 |
private-kernel-tail |
214.04 MB |
213.94 MB |
1.00 |
rollup-base-private |
1350 MB |
1350 MB |
1 |
rollup-base-public |
1400 MB |
1400 MB |
1 |
rollup-block-root-empty |
1010 MB |
1010 MB |
1 |
rollup-block-root-single-tx |
9690 MB |
9690 MB |
1 |
rollup-block-root |
9690 MB |
9690 MB |
1 |
rollup-merge |
330.61 MB |
330.59 MB |
1.00 |
rollup-root |
341.38 MB |
341.33 MB |
1.00 |
semaphore_depth_10 |
104.75 MB |
104.73 MB |
1.00 |
sha512_100_bytes |
233.01 MB |
232.98 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.
Test Suite Duration
Benchmark suite | Current: ad2a326 | Previous: b544e60 | Ratio |
---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr |
97 s |
99 s |
0.98 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts |
111 s |
109 s |
1.02 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
160 s |
188 s |
0.85 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
209 s |
208 s |
1.00 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib |
34 s |
33 s |
1.03 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
629 s |
593 s |
1.06 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
95 s |
93 s |
1.02 |
test_report_noir-lang_noir-bignum_ |
134 s |
136 s |
0.99 |
test_report_noir-lang_noir_bigcurve_ |
347 s |
279 s |
1.24 |
test_report_noir-lang_sha256_ |
15 s |
15 s |
1 |
test_report_noir-lang_sha512_ |
12 s |
15 s |
0.80 |
test_report_zkpassport_noir-ecdsa_ |
1 s |
2 s |
0.50 |
test_report_zkpassport_noir_rsa_ |
2 s |
2 s |
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.
Looks great!
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: make Ord for slices lexicographic (elements first, then length) (noir-lang/noir#9555) chore(ssa): Refactor `unrolling` (noir-lang/noir#9653) chore(docs): Update dependency page's examples (noir-lang/noir#9634) fix(ssa): Constant fold Brillig calls using the SSA interpreter (noir-lang/noir#9655) chore: LICM refactors (noir-lang/noir#9642) chore: add test for trait bound on implementing type (noir-lang/noir#9652) chore: pass `DataFlowGraph` instead of `Function` as arg (noir-lang/noir#9656) feat: Group one audit tests (noir-lang/noir#9445) fix(expand): better handling of dereferences (again) (noir-lang/noir#9654) feat(mem2reg): address last known value is independent of its aliases (take three) (noir-lang/noir#9633) chore: remove handling for slice arguments to MSM (noir-lang/noir#9648) fix: validate binary operations which do not allow fields (noir-lang/noir#9649) 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 fix: make Ord for slices lexicographic (elements first, then length) (noir-lang/noir#9555) chore(ssa): Refactor `unrolling` (noir-lang/noir#9653) chore(docs): Update dependency page's examples (noir-lang/noir#9634) fix(ssa): Constant fold Brillig calls using the SSA interpreter (noir-lang/noir#9655) chore: LICM refactors (noir-lang/noir#9642) chore: add test for trait bound on implementing type (noir-lang/noir#9652) chore: pass `DataFlowGraph` instead of `Function` as arg (noir-lang/noir#9656) feat: Group one audit tests (noir-lang/noir#9445) fix(expand): better handling of dereferences (again) (noir-lang/noir#9654) feat(mem2reg): address last known value is independent of its aliases (take three) (noir-lang/noir#9633) chore: remove handling for slice arguments to MSM (noir-lang/noir#9648) fix: validate binary operations which do not allow fields (noir-lang/noir#9649) END_COMMIT_OVERRIDE
Description
Problem*
Alternative to #9365
Resolves #9015
Resolves #9097
Resolves #9484 (comment) (same error as #9097)
Resolves a panic from a Noir sync when attempting to debug log an array value (the same error as #9097)
Let's take the snippet from #9097:
We can see the following SSA when we fold constants with Brillig:
Details
We can see that one Brillig function was folding into
f0
. However, the print remains. However, the Brillig context structure contains all the artifacts already has a set mapping for function ids to Brillig artifacts. Thus, when we remove functions after Brillig gen and change their IDs (e.g., like when normalizing) we end up fetching an artifact for the wrong function when it comes times to generate an entry point.Summary*
Alternative to #9365. I realized we were already cloning all Brillig functions in
fold_constants_with_brillig
. In this version I simply changed the interface of the SSA interpreter to allow passing a mapping of functions instead of the entire SSA. This enables us to fold Brillig calls as part ofconstant_folding
which avoids some of the regressions (both in bytecode size and compilation time) from #9365. Folding Brillig calls as part ofconstant_folding
allows us to access new constants that get exposed during the execution of the pass.This issue could have been fixed for #9097 by simply updating the IDs point to the Brillig artifacts as part of normalization. However, this felt brittle and I felt the problem laid out above still feels like it could come up elsewhere.
Aside the benefit of avoiding extra Brillig compilation, switching to using the SSA interpreter for constant folding Brillig entry points also avoids these kinds of situations where Brillig context needs to be updated due to secondary SSA passes post Brillig gen.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmt
on default settings.