chore(acir): validate predicate use against requires_acir_gen_predicate - #13517
Open
asterite wants to merge 1 commit into
Open
chore(acir): validate predicate use against requires_acir_gen_predicate#13517asterite wants to merge 1 commit into
asterite wants to merge 1 commit into
Conversation
SSA passes (remove_enable_side_effects, constant folding, LICM, DIE) rely on Instruction::requires_acir_gen_predicate to decide when an instruction may be moved or deduplicated across EnableSideEffectsIf boundaries, but nothing checked that ACIR gen's actual reads of the side-effects predicate agree with it. A lowering that consults the predicate for an instruction reporting false would be silently miscompiled by those passes; the reverse direction hides stale over-approximations and missing guards. Every read of current_side_effects_enabled_var during instruction conversion now goes through read_predicate(), which records the read, and lowering fast paths that deliberately skip the predicate acknowledge it explicitly via predicate_not_needed(). After each instruction converts, an assertion checks the recorded flag equals requires_acir_gen_predicate in both directions (EnableSideEffectsIf excluded: it writes the predicate rather than reading it). Behavior-preserving restructures needed for the equality to hold: - Blackbox intrinsic calls fetch the predicate only for RecursiveAggregation, the one blackbox that consumes it; all others received and ignored it. - handle_disabled_array_operation checks the statically-safe-read early return before inspecting the predicate. - convert_array_operation_inputs no longer inspects the predicate for plain reads; only stores are predicated there. Deliberate-skip acknowledgments cover: compile-time constant-index reads at indices is_safe_index cannot see (constant indices into vectors), the constant-offset fast path in get_flattened_index (gated callers only), push_back with a compile-time-known length, pops of a known nonzero constant length, and statically safe insert/remove indices. Also rewrites the stale comment on VectorPopFront/VectorPopBack in requires_acir_gen_predicate: since the empty-vector handling those pops do consult the predicate (empty-backing-store assertion, predicated emptiness check, gated index), reporting true is genuinely required, not a pretence. Verified: full nargo_cli execute suite (9334 tests), stdlib-tests, and all noirc_evaluator unit tests pass with the assertion active; aztec-packages protocol circuits compile clean with compile-time impact within noise (<=1%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ACVM Benchmarks
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
purely_sequential_opcodes |
174487 ns/iter (± 209) |
132209 ns/iter (± 525) |
1.32 |
perfectly_parallel_opcodes |
142466 ns/iter (± 4255) |
115393 ns/iter (± 1158) |
1.23 |
perfectly_parallel_batch_inversion_opcodes |
2651490 ns/iter (± 6333) |
3042696 ns/iter (± 6998) |
0.87 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Elaboration Time
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
1.296 s |
1.302 s |
1.00 |
private-kernel-reset-tail |
1.27 s |
1.05 s |
1.21 |
private-kernel-reset |
1.254 s |
1.294 s |
0.97 |
rollup-block-root-first-empty-tx |
2.264 s |
2.268 s |
1.00 |
rollup-block-root-single-tx |
2.07 s |
2.09 s |
0.99 |
rollup-block-root |
2.13 s |
2.15 s |
0.99 |
rollup-checkpoint-merge |
2.182 s |
2.092 s |
1.04 |
rollup-checkpoint-root-single-block |
2.15 s |
2.24 s |
0.96 |
rollup-checkpoint-root |
1.8 s |
2.09 s |
0.86 |
rollup-root |
2.114 s |
1.434 s |
1.47 |
rollup-tx-base-private |
2.138 s |
2.074 s |
1.03 |
rollup-tx-base-public |
1.64 s |
2.244 s |
0.73 |
rollup-tx-merge |
2.124 s |
2.232 s |
0.95 |
semaphore-depth-10 |
0.29 s |
0.238 s |
1.22 |
sha512-100-bytes |
0.259 s |
0.238 s |
1.09 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Compilation Time
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
6.642 s |
6.304 s |
1.05 |
private-kernel-reset-tail |
221 s |
168 s |
1.32 |
private-kernel-reset |
13.92 s |
13.66 s |
1.02 |
rollup-block-root-first-empty-tx |
2.652 s |
2.662 s |
1.00 |
rollup-block-root-single-tx |
2.39 s |
2.42 s |
0.99 |
rollup-block-root |
2.56 s |
2.59 s |
0.99 |
rollup-checkpoint-merge |
2.578 s |
2.464 s |
1.05 |
rollup-checkpoint-root-single-block |
267 s |
321 s |
0.83 |
rollup-checkpoint-root |
235 s |
248 s |
0.95 |
rollup-root |
2.6 s |
1.792 s |
1.45 |
rollup-tx-base-private |
17.56 s |
15.58 s |
1.13 |
rollup-tx-base-public |
37.32 s |
53.8 s |
0.69 |
rollup-tx-merge |
2.488 s |
2.614 s |
0.95 |
semaphore-depth-10 |
1.018 s |
0.793 s |
1.28 |
sha512-100-bytes |
1.702 s |
1.778 s |
0.96 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Brillig Artifact Size
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
823.4 KB |
823.4 KB |
1 |
private-kernel-reset-tail |
904.1 KB |
904.1 KB |
1 |
private-kernel-reset |
666.7 KB |
666.7 KB |
1 |
rollup-block-root-first-empty-tx |
324.7 KB |
324.7 KB |
1 |
rollup-block-root-single-tx |
328.6 KB |
328.6 KB |
1 |
rollup-block-root |
390.5 KB |
390.5 KB |
1 |
rollup-checkpoint-merge |
312.6 KB |
312.6 KB |
1 |
rollup-checkpoint-root-single-block |
690.6 KB |
690.6 KB |
1 |
rollup-checkpoint-root |
731.2 KB |
731.2 KB |
1 |
rollup-root |
489.9 KB |
489.9 KB |
1 |
rollup-tx-base-private |
691.3 KB |
691.3 KB |
1 |
rollup-tx-base-public |
854.6 KB |
854.6 KB |
1 |
rollup-tx-merge |
244.6 KB |
244.6 KB |
1 |
semaphore-depth-10 |
2073.7 KB |
2073.7 KB |
1 |
sha512-100-bytes |
151.4 KB |
151.4 KB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Artifact Size
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
1105.9 KB |
1105.9 KB |
1 |
private-kernel-reset-tail |
2681.5 KB |
2681.5 KB |
1 |
private-kernel-reset |
2113.5 KB |
2113.5 KB |
1 |
rollup-block-root-first-empty-tx |
276.9 KB |
276.9 KB |
1 |
rollup-block-root-single-tx |
280.4 KB |
280.4 KB |
1 |
rollup-block-root |
337.7 KB |
337.7 KB |
1 |
rollup-checkpoint-merge |
436.2 KB |
436.2 KB |
1 |
rollup-checkpoint-root-single-block |
30746.1 KB |
30746.1 KB |
1 |
rollup-checkpoint-root |
30777.3 KB |
30777.3 KB |
1 |
rollup-root |
470.8 KB |
470.8 KB |
1 |
rollup-tx-base-private |
4598.9 KB |
4598.9 KB |
1 |
rollup-tx-base-public |
5167.3 KB |
5167.3 KB |
1 |
rollup-tx-merge |
209.1 KB |
209.1 KB |
1 |
semaphore-depth-10 |
502.9 KB |
502.9 KB |
1 |
sha512-100-bytes |
449.9 KB |
449.9 KB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Execution Time
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.039 s |
0.039 s |
1 |
private-kernel-reset-tail |
0.173 s |
0.143 s |
1.21 |
private-kernel-reset |
0.126 s |
0.117 s |
1.08 |
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.004 s |
0.004 s |
1 |
rollup-checkpoint-merge |
0.002 s |
0.002 s |
1 |
rollup-checkpoint-root-single-block |
9.346 s |
7.73 s |
1.21 |
rollup-checkpoint-root |
8.844 s |
9.214 s |
0.96 |
rollup-root |
0.003 s |
0.003 s |
1 |
rollup-tx-base-private |
0.304 s |
0.28 s |
1.09 |
rollup-tx-base-public |
0.212 s |
0.238 s |
0.89 |
rollup-tx-merge |
0.002 s |
0.002 s |
1 |
semaphore-depth-10 |
0.009 s |
0.008 s |
1.13 |
sha512-100-bytes |
0.042 s |
0.062 s |
0.68 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Brillig Compilation Time
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
1.914 s |
1.918 s |
1.00 |
private-kernel-reset-tail |
2.4 s |
1.94 s |
1.24 |
private-kernel-reset |
1.854 s |
1.928 s |
0.96 |
rollup-block-root-first-empty-tx |
2.578 s |
2.664 s |
0.97 |
rollup-block-root-single-tx |
2.38 s |
2.44 s |
0.98 |
rollup-block-root |
2.47 s |
2.49 s |
0.99 |
rollup-checkpoint-merge |
2.49 s |
2.394 s |
1.04 |
rollup-checkpoint-root-single-block |
3.83 s |
4.65 s |
0.82 |
rollup-checkpoint-root |
3.35 s |
3.84 s |
0.87 |
rollup-root |
2.486 s |
1.712 s |
1.45 |
rollup-tx-base-private |
2.67 s |
2.634 s |
1.01 |
rollup-tx-base-public |
2.13 s |
2.922 s |
0.73 |
rollup-tx-merge |
2.428 s |
2.56 s |
0.95 |
semaphore-depth-10 |
0.432 s |
0.357 s |
1.21 |
sha512-100-bytes |
0.371 s |
0.346 s |
1.07 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Opcode count
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
18980 opcodes |
18980 opcodes |
1 |
private-kernel-reset-tail |
96712 opcodes |
96712 opcodes |
1 |
private-kernel-reset |
80908 opcodes |
80908 opcodes |
1 |
rollup-block-root-first-empty-tx |
1038 opcodes |
1038 opcodes |
1 |
rollup-block-root-single-tx |
922 opcodes |
922 opcodes |
1 |
rollup-block-root |
2105 opcodes |
2105 opcodes |
1 |
rollup-checkpoint-merge |
1435 opcodes |
1435 opcodes |
1 |
rollup-checkpoint-root-single-block |
1388565 opcodes |
1388565 opcodes |
1 |
rollup-checkpoint-root |
1389745 opcodes |
1389745 opcodes |
1 |
rollup-root |
1797 opcodes |
1797 opcodes |
1 |
rollup-tx-base-private |
245177 opcodes |
245177 opcodes |
1 |
rollup-tx-base-public |
280542 opcodes |
280542 opcodes |
1 |
rollup-tx-merge |
1303 opcodes |
1303 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.
Contributor
There was a problem hiding this comment.
Brillig Execution Time
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
0.03 s |
0.03 s |
1 |
private-kernel-reset-tail |
0.045 s |
0.035 s |
1.29 |
private-kernel-reset |
0.008 s |
0.008 s |
1 |
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.003 s |
0.003 s |
1 |
rollup-checkpoint-merge |
0.001 s |
0.001 s |
1 |
rollup-root |
0.002 s |
0.002 s |
1 |
rollup-tx-base-private |
0.047 s |
0.044 s |
1.07 |
rollup-tx-base-public |
0.025 s |
0.033 s |
0.76 |
rollup-tx-merge |
0.001 s |
0.002 s |
0.50 |
semaphore-depth-10 |
0.007 s |
0.006 s |
1.17 |
sha512-100-bytes |
0.013 s |
0.012 s |
1.08 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Compilation Memory
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
313.85 MB |
313.85 MB |
1 |
private-kernel-reset-tail |
548.48 MB |
548.48 MB |
1 |
private-kernel-reset |
524.69 MB |
524.69 MB |
1 |
rollup-block-root-first-empty-tx |
361.47 MB |
361.47 MB |
1 |
rollup-block-root-single-tx |
361.47 MB |
361.47 MB |
1 |
rollup-block-root |
362.34 MB |
362.34 MB |
1 |
rollup-checkpoint-merge |
361.47 MB |
361.47 MB |
1 |
rollup-checkpoint-root-single-block |
5800 MB |
5800 MB |
1 |
rollup-checkpoint-root |
5800 MB |
5800 MB |
1 |
rollup-root |
363.06 MB |
363.06 MB |
1 |
rollup-tx-base-private |
870.83 MB |
870.83 MB |
1 |
rollup-tx-base-public |
1340 MB |
1340 MB |
1 |
rollup-tx-merge |
361.47 MB |
361.47 MB |
1 |
semaphore_depth_10 |
103.76 MB |
103.76 MB |
1 |
sha512_100_bytes |
149.12 MB |
149.12 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Execution Memory
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
private-kernel-inner |
294.22 MB |
294.22 MB |
1 |
private-kernel-reset-tail |
335.23 MB |
335.23 MB |
1 |
private-kernel-reset |
321.54 MB |
321.54 MB |
1 |
rollup-block-root |
362.55 MB |
362.55 MB |
1 |
rollup-checkpoint-merge |
362.71 MB |
362.71 MB |
1 |
rollup-checkpoint-root-single-block |
1280 MB |
1280 MB |
1 |
rollup-checkpoint-root |
1280 MB |
1280 MB |
1 |
rollup-root |
362.95 MB |
362.95 MB |
1 |
rollup-tx-base-private |
468.33 MB |
468.33 MB |
1 |
rollup-tx-base-public |
480.69 MB |
480.69 MB |
1 |
rollup-tx-merge |
362.19 MB |
362.19 MB |
1 |
semaphore_depth_10 |
84.1 MB |
84.1 MB |
1 |
sha512_100_bytes |
77.92 MB |
77.92 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Test Suite Duration
Details
| Benchmark suite | Current: 0043f7e | Previous: 9dfde92 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_fnd_noir-protocol-circuits_crates_blob |
274 s |
292 s |
0.94 |
test_report_AztecProtocol_aztec-packages_noir-projects_fnd_noir-protocol-circuits_crates_private-kernel-lib |
1003 s |
971 s |
1.03 |
test_report_AztecProtocol_aztec-packages_noir-projects_fnd_noir-protocol-circuits_crates_types |
177 s |
170 s |
1.04 |
test_report_AztecProtocol_aztec-packages_noir-projects_labs_aztec-nr |
846 s |
818 s |
1.03 |
test_report_AztecProtocol_aztec-packages_noir-projects_labs_noir-contracts |
515 s |
493 s |
1.04 |
test_report_noir-lang_noir-bignum_ |
140 s |
171 s |
0.82 |
test_report_noir-lang_noir_bigcurve_ |
354 s |
335 s |
1.06 |
test_report_noir-lang_noir_json_parser_ |
15 s |
13 s |
1.15 |
test_report_noir-lang_sha256_ |
34 s |
33 s |
1.03 |
test_report_noir-lang_sha512_ |
13 s |
13 s |
1 |
test_report_zkpassport_noir-ecdsa_ |
3 s |
4 s |
0.75 |
test_report_zkpassport_noir_rsa_ |
2 s |
3 s |
0.67 |
This comment was automatically generated by workflow using github-action-benchmark.
Collaborator
Author
|
@TomAFrench I don't know if this is what you had in mind. With this at least there can never be a mismatch between us saying "yes, this needs the predicate in ACIR gen" and ACIR gen not using it, and the other way around too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem
Instruction::requires_acir_gen_predicateand ACIR gen's actual use of the side-effects predicate had no checked relationship. SSA passes (remove_enable_side_effects, constant folding dedup, LICM, DIE) rely on this method to decide when an instruction may be moved or deduplicated acrossEnableSideEffectsIfboundaries. If a lowering consultscurrent_side_effects_enabled_varfor an instruction reportingfalse, those passes can move it under the wrong predicate — a silent miscompile (past bugs onVectorPushBackandRecursiveAggregationwere exactly this shape). The reverse direction (reportstruebut never reads) hides stale over-approximations and forgotten guards. The method was maintained by hand as a mirror of the lowering code, with drift caught only by luck: the comment onVectorPopFront/VectorPopBackclaiming they "don't use the side effects predicate" was already false when it was written.Summary
Every read of the predicate during instruction conversion now goes through
Context::read_predicate(), which records the read (transitive reads, e.g. throughget_flattened_index, are covered automatically). Lowering fast paths that deliberately skip the predicate acknowledge it explicitly withContext::predicate_not_needed(reason). After converting each instruction, an assertion checks the recorded flag equalsrequires_acir_gen_predicatein both directions (EnableSideEffectsIfexcluded: it writes the predicate rather than reading it). Any future drift between the method and the lowerings becomes an ICE on the first program that exercises it instead of a latent soundness bug.Behavior-preserving restructures needed for the equality to hold:
RecursiveAggregation— the one blackbox that consumes it; all others received and ignored it.handle_disabled_array_operationchecks the statically-safe-read early return before inspecting the predicate.convert_array_operation_inputsno longer inspects the predicate for plain reads; only stores are predicated there.Deliberate-skip acknowledgments cover: compile-time constant-index reads at indices
is_safe_indexcannot see (in practice: constant indices into vectors — 15 test programs exercise this), the constant-offset fast path inget_flattened_index(gated callers only),push_backwith a compile-time-known length, pops of a known nonzero constant length, and statically safe insert/remove indices.Also rewrites the stale
VectorPop*comment inrequires_acir_gen_predicate: those pops genuinely consult the predicate (empty-backing-store assertion, predicated emptiness check, gated decremented index), sotrueis required, not a pretence.Verification
nargo_cliexecute suite (9334 tests), stdlib-tests, and allnoirc_evaluatorunit tests pass with the assertion active — zero firings across the corpus in either direction.RecursiveAggregationin private-kernel-inner) compile clean; interleaved master-vs-branch timing of parity-base, private-kernel-inner, rollup-tx-merge and rollup-tx-base-private shows impact within run-to-run noise (≤1%). Labelbench-showadded to confirm on CI benchmarks.🤖 Generated with Claude Code