fix(ssa): do not fold ArraySet to MakeArray in simplifier#11659
fix(ssa): do not fold ArraySet to MakeArray in simplifier#11659TomAFrench merged 37 commits intomasterfrom
Conversation
The DFG simplifier was rewriting predicate-dependent ArraySet instructions into predicate-independent MakeArray when the base array and index were constants. This erased the EnableSideEffectsIf guard that ACIR relies on, making conditional writes unconditional and allowing incorrect witness values to satisfy constraints. Remove the ArraySet→MakeArray constant fold since the simplifier has no access to the current side-effects predicate and cannot determine whether the fold is safe.
…_set NOIR-17 was a false positive — the simplifier already correctly refuses to optimize array_get through a same-index predicated array_set when it lacks predicate context (side_effects = None). Add a test to ensure this behavior is preserved.
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: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
rollup-checkpoint-merge |
0.003 s |
0.002 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Artifact Size'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 396508f | Previous: 28c3b5f | Ratio |
|---|---|---|---|
rollup-tx-base-public |
5696.8 KB |
4693.9 KB |
1.21 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 29af7df | Previous: 128c6eb | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
2.24 s |
1.74 s |
1.29 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Opcode count'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 396508f | Previous: 28c3b5f | Ratio |
|---|---|---|---|
rollup-checkpoint-root-single-block |
1560093 opcodes |
1387956 opcodes |
1.12 |
rollup-checkpoint-root |
1561286 opcodes |
1389150 opcodes |
1.12 |
rollup-tx-base-private |
358504 opcodes |
299181 opcodes |
1.20 |
rollup-tx-base-public |
314113 opcodes |
254789 opcodes |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 9081cf0 | Previous: 824f568 | Ratio |
|---|---|---|---|
rollup-tx-base-private |
1700 MB |
1070 MB |
1.59 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 9081cf0 | Previous: 824f568 | Ratio |
|---|---|---|---|
rollup-tx-base-private |
688.35 MB |
527.34 MB |
1.31 |
rollup-tx-base-public |
556.46 MB |
462.99 MB |
1.20 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
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: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
test_report_noir-lang_sha512_ |
14 s |
11 s |
1.27 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Brillig functions do not use side-effects predicates, so the ArraySet-to-MakeArray simplification is safe there. Guard the optimization with a runtime check instead of disabling it entirely.
asterite
left a comment
There was a problem hiding this comment.
Looks good!
Should we capture a follow-up issue to perform this optimization as a pass, if we find that the predicate of the MakeArray instruction is the same as the one for the ArraySet? I guess it could go with the existing SSA pass that does this for ArrayGet, and it could maybe revert these regressions.
When I implemented this for ArrayGet I didn't realize this was only an issue in ACIR, so maybe we could also change that code to take that into account (though I don't think it'll further optimize things).
|
Ah, lemme take another look at #11586 to see if I can tie this into your changes a bit more. |
…dedicated pass Move the constant-array ArraySet folding out of the simplifier (which lacks predicate context) into the array_set_optimization pass. Extract mutable_array_set as a separate module and fix test expectations.
There was a problem hiding this comment.
ACVM Benchmarks
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
purely_sequential_opcodes |
251600 ns/iter (± 615) |
251446 ns/iter (± 484) |
1.00 |
perfectly_parallel_opcodes |
220410 ns/iter (± 4690) |
221480 ns/iter (± 6603) |
1.00 |
perfectly_parallel_batch_inversion_opcodes |
2741913 ns/iter (± 2640) |
2745617 ns/iter (± 6703) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
|
I've pulled the renaming of the old |
There was a problem hiding this comment.
Elaboration Time
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.993 s |
0.932 s |
1.07 |
private-kernel-reset |
0.92 s |
0.984 s |
0.93 |
private-kernel-tail |
0.994 s |
0.929 s |
1.07 |
rollup-block-root-first-empty-tx |
1.464 s |
1.48 s |
0.99 |
rollup-block-root-single-tx |
1.4 s |
1.36 s |
1.03 |
rollup-block-root |
1.38 s |
1.42 s |
0.97 |
rollup-checkpoint-merge |
1.468 s |
1.396 s |
1.05 |
rollup-checkpoint-root-single-block |
1.43 s |
1.36 s |
1.05 |
rollup-checkpoint-root |
1.4 s |
1.35 s |
1.04 |
rollup-root |
1.414 s |
1.392 s |
1.02 |
rollup-tx-base-private |
1.394 s |
1.388 s |
1.00 |
rollup-tx-base-public |
1.388 s |
1.396 s |
0.99 |
rollup-tx-merge |
1.398 s |
1.398 s |
1 |
semaphore-depth-10 |
0.157 s |
0.147 s |
1.07 |
sha512-100-bytes |
0.133 s |
0.136 s |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Opcode count
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
19357 opcodes |
19357 opcodes |
1 |
private-kernel-reset |
85604 opcodes |
85604 opcodes |
1 |
private-kernel-tail |
8739 opcodes |
8739 opcodes |
1 |
rollup-block-root-first-empty-tx |
1088 opcodes |
1082 opcodes |
1.01 |
rollup-block-root-single-tx |
975 opcodes |
969 opcodes |
1.01 |
rollup-block-root |
2177 opcodes |
2171 opcodes |
1.00 |
rollup-checkpoint-merge |
1271 opcodes |
1271 opcodes |
1 |
rollup-checkpoint-root-single-block |
1412521 opcodes |
1387956 opcodes |
1.02 |
rollup-checkpoint-root |
1413715 opcodes |
1389150 opcodes |
1.02 |
rollup-root |
1525 opcodes |
1525 opcodes |
1 |
rollup-tx-base-private |
307656 opcodes |
299181 opcodes |
1.03 |
rollup-tx-base-public |
263264 opcodes |
254789 opcodes |
1.03 |
rollup-tx-merge |
1302 opcodes |
1302 opcodes |
1 |
semaphore-depth-10 |
5699 opcodes |
5699 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.
Artifact Size
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
993.8 KB |
993.8 KB |
1 |
private-kernel-reset |
2200.5 KB |
2200.5 KB |
1 |
private-kernel-tail |
489.9 KB |
490 KB |
1.00 |
rollup-block-root-first-empty-tx |
230.9 KB |
230.7 KB |
1.00 |
rollup-block-root-single-tx |
234.3 KB |
234.2 KB |
1.00 |
rollup-block-root |
306.4 KB |
306.3 KB |
1.00 |
rollup-checkpoint-merge |
365.2 KB |
365.2 KB |
1 |
rollup-checkpoint-root-single-block |
31154.9 KB |
30782.4 KB |
1.01 |
rollup-checkpoint-root |
31207.3 KB |
30834.5 KB |
1.01 |
rollup-root |
389.8 KB |
389.8 KB |
1 |
rollup-tx-base-private |
5515.8 KB |
5384.3 KB |
1.02 |
rollup-tx-base-public |
4813.9 KB |
4689.9 KB |
1.03 |
rollup-tx-merge |
178.5 KB |
178.5 KB |
1 |
semaphore-depth-10 |
488.6 KB |
488.6 KB |
1 |
sha512-100-bytes |
473.7 KB |
473.7 KB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Brillig Compilation Time
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
1.54 s |
1.5 s |
1.03 |
private-kernel-reset |
1.522 s |
1.578 s |
0.96 |
private-kernel-tail |
1.378 s |
1.294 s |
1.06 |
rollup-block-root-first-empty-tx |
1.782 s |
1.892 s |
0.94 |
rollup-block-root-single-tx |
1.7 s |
1.69 s |
1.01 |
rollup-block-root |
1.74 s |
1.77 s |
0.98 |
rollup-checkpoint-merge |
1.796 s |
1.7 s |
1.06 |
rollup-checkpoint-root-single-block |
2.29 s |
2.24 s |
1.02 |
rollup-checkpoint-root |
2.33 s |
2.3 s |
1.01 |
rollup-root |
1.85 s |
1.756 s |
1.05 |
rollup-tx-base-private |
2.078 s |
1.872 s |
1.11 |
rollup-tx-base-public |
1.962 s |
2.04 s |
0.96 |
rollup-tx-merge |
1.702 s |
1.702 s |
1 |
semaphore-depth-10 |
0.297 s |
0.282 s |
1.05 |
sha512-100-bytes |
0.244 s |
0.243 s |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Time
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
2.97 s |
2.638 s |
1.13 |
private-kernel-reset |
7.948 s |
8.434 s |
0.94 |
private-kernel-tail |
2.57 s |
2.622 s |
0.98 |
rollup-block-root-first-empty-tx |
1.854 s |
1.86 s |
1.00 |
rollup-block-root-single-tx |
1.75 s |
1.7 s |
1.03 |
rollup-block-root |
1.8 s |
1.83 s |
0.98 |
rollup-checkpoint-merge |
1.856 s |
1.778 s |
1.04 |
rollup-checkpoint-root-single-block |
332 s |
299 s |
1.11 |
rollup-checkpoint-root |
316 s |
307 s |
1.03 |
rollup-root |
1.86 s |
1.806 s |
1.03 |
rollup-tx-base-private |
20.64 s |
19.06 s |
1.08 |
rollup-tx-base-public |
110 s |
102.8 s |
1.07 |
rollup-tx-merge |
1.756 s |
1.75 s |
1.00 |
semaphore-depth-10 |
1.193 s |
1.118 s |
1.07 |
sha512-100-bytes |
1.719 s |
2.051 s |
0.84 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Time
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.04 s |
0.041 s |
0.98 |
private-kernel-reset |
0.174 s |
0.176 s |
0.99 |
private-kernel-tail |
0.009 s |
0.008 s |
1.13 |
rollup-block-root-first-empty-tx |
0.003 s |
0.003 s |
1 |
rollup-block-root-single-tx |
0.003 s |
0.003 s |
1 |
rollup-block-root |
0.003 s |
0.004 s |
0.75 |
rollup-checkpoint-merge |
0.003 s |
0.002 s |
1.50 |
rollup-checkpoint-root-single-block |
10.8 s |
10.7 s |
1.01 |
rollup-checkpoint-root |
9.85 s |
10.4 s |
0.95 |
rollup-root |
0.003 s |
0.003 s |
1 |
rollup-tx-base-private |
0.336 s |
0.332 s |
1.01 |
rollup-tx-base-public |
0.251 s |
0.252 s |
1.00 |
rollup-tx-merge |
0.002 s |
0.002 s |
1 |
semaphore-depth-10 |
0.009 s |
0.009 s |
1 |
sha512-100-bytes |
0.058 s |
0.083 s |
0.70 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Brillig Artifact Size
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
766 KB |
766 KB |
1 |
private-kernel-reset |
706.3 KB |
706.3 KB |
1 |
private-kernel-tail |
338.6 KB |
338.6 KB |
1 |
rollup-block-root-first-empty-tx |
272 KB |
272 KB |
1 |
rollup-block-root-single-tx |
276.3 KB |
276.3 KB |
1 |
rollup-block-root |
337.3 KB |
337.3 KB |
1 |
rollup-checkpoint-merge |
268.4 KB |
268.4 KB |
1 |
rollup-checkpoint-root-single-block |
539.1 KB |
539.1 KB |
1 |
rollup-checkpoint-root |
581.6 KB |
581.6 KB |
1 |
rollup-root |
405.8 KB |
405.8 KB |
1 |
rollup-tx-base-private |
636.4 KB |
636.4 KB |
1 |
rollup-tx-base-public |
802.4 KB |
802.4 KB |
1 |
rollup-tx-merge |
202.8 KB |
202.8 KB |
1 |
semaphore-depth-10 |
2067.4 KB |
2067.4 KB |
1 |
sha512-100-bytes |
163.5 KB |
163.5 KB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Brillig Execution Time
Details
| Benchmark suite | Current: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.031 s |
0.032 s |
0.97 |
private-kernel-reset |
0.05 s |
0.052 s |
0.96 |
private-kernel-tail |
0.004 s |
0.005 s |
0.80 |
rollup-block-root-first-empty-tx |
0.003 s |
0.003 s |
1 |
rollup-block-root-single-tx |
0.002 s |
0.002 s |
1 |
rollup-block-root |
0.002 s |
0.003 s |
0.67 |
rollup-checkpoint-merge |
0.001 s |
0.001 s |
1 |
rollup-root |
0.002 s |
0.002 s |
1 |
rollup-tx-base-private |
0.028 s |
0.028 s |
1 |
rollup-tx-base-public |
0.031 s |
0.032 s |
0.97 |
rollup-tx-merge |
0.001 s |
0.001 s |
1 |
semaphore-depth-10 |
0.023 s |
0.023 s |
1 |
sha512-100-bytes |
0.016 s |
0.014 s |
1.14 |
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: 75f893b | Previous: 001f104 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr |
246 s |
233 s |
1.06 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts |
183 s |
178 s |
1.03 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
239 s |
269 s |
0.89 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
492 s |
464 s |
1.06 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
162 s |
156 s |
1.04 |
test_report_noir-lang_noir-bignum_ |
207 s |
186 s |
1.11 |
test_report_noir-lang_noir_bigcurve_ |
374 s |
335 s |
1.12 |
test_report_noir-lang_sha256_ |
18 s |
18 s |
1 |
test_report_noir-lang_sha512_ |
14 s |
11 s |
1.27 |
test_report_zkpassport_noir-ecdsa_ |
1 s |
1 s |
1 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
|
My experiment with merging is in #11760 At the same time we confirmed with @guipublic that assigning incorrect values to the witnesses representing the inputs of We were wondering with @asterite whether it is even possible to construct a program that would make this unwanted side effect of unconditionally replacing the result of a disabled array_set even possible, or will the value merger always mask the results. |
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: a012f4a | Previous: 28c3b5f | Ratio |
|---|---|---|---|
perfectly_parallel_batch_inversion_opcodes |
2746089 ns/iter (± 2902) |
2209309 ns/iter (± 4639) |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 396508f | Previous: 28c3b5f | Ratio |
|---|---|---|---|
perfectly_parallel_batch_inversion_opcodes |
2743531 ns/iter (± 2148) |
2209309 ns/iter (± 4639) |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
One thing I'm noticing is that when we compile But when we compile it in this branch we get bugs (in addition to the warning above): I wonder if those bugs are legitimate or this PR introduces a bug... Then it seems #11773 doesn't fix the regression in |
It is true that the results of the call it points at are not constrained against any input, nor are they returned: noir/noir_stdlib/src/field/bn254.nr Lines 33 to 48 in 8ed9293 |
This is only half true as the range constraints are expected to constrain the value of |
|
It is also true that noir/test_programs/execution_success/hashmap/src/main.nr Lines 160 to 166 in 8ed9293 Maybe this is whey the |
It looks like the method that considers things connected only looks for return values and inputs: Where it connects range constraints, Ah no, they are actually not connected 🙈 : |
|
It seems the hashmap regression is gone! 🎉 And then #11773 optimized the case that Aztec-Packages was bumping into. |
Before it was gone, the SSA could not eliminate the Brillig calls as constants, the |
Summary
ArraySetinstructions into predicate-independentMakeArraywhen the base array and index were constants. This erased theEnableSideEffectsIfguard that ACIR relies on, making conditional writes unconditional.ArraySet→MakeArrayconstant fold since the simplifier has no access to the current side-effects predicate and cannot determine whether the fold is safe.array_getthrough a predicatedarray_setis also not incorrectly simplified (already handled correctly).Test plan
array_set_constant_folding_must_respect_side_effects_predicateverifies simplification preserves semantics under both enabled and disabled side effectsarray_get_not_simplified_through_predicated_array_setconfirms existing conservative behaviornoirc_evaluatortests passsimplifies_array_get_from_previous_array_set_with_make_arrayandrevisit_block_which_dominates_cache