feat(ssa): Following an always failing binary, replace instructions with defaults until the next predicate#9211
Conversation
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: 0a24d9c | Previous: e7a98f2 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
132 s |
105 s |
1.26 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
Fuzzing found an example in https://github.com/noir-lang/noir/actions/runs/16296162751/job/46019184144?pr=9211 where the minimal pipeline returns a non-zero value, while the full return 0. It minimises to this: global G_A: [Field] = &[];
fn main(a: u32) -> pub Field {
match a {
4166865152 => G_A[(102921721_u32 % G_A.len())],
2676856374 => (a as Field),
_ => -339696707650690765565390531789020390020,
}
}The culprit seems to be the "EnableSideEffectsIf removal" pass, which follows this stage, and removes all interim After Simplify conditionals for unconstrained (1) (step 24):
g0 = u32 0
g1 = make_array [] : [Field]
acir(inline) predicate_pure fn main f0 {
b0(v2: u32):
v4 = eq v2, u32 4166865152
enable_side_effects v4
v6 = mod u32 102921721, u32 0 // src/main.nr:4:28
constrain u1 0 == v4, "Index out of bounds" // src/main.nr:4:28
v8 = make_array [] : [Field] // src/main.nr:4:28
v9 = array_get v8, index v6 -> Field // src/main.nr:4:23
v10 = not v4
enable_side_effects v10
v12 = eq v2, u32 2676856374 // src/main.nr:4:23
v13 = unchecked_mul v10, v12 // src/main.nr:4:23
enable_side_effects v13 // src/main.nr:4:23
v14 = cast v2 as Field // src/main.nr:5:24
v15 = not v12 // src/main.nr:4:23
v16 = unchecked_mul v10, v15 // src/main.nr:4:23
enable_side_effects v10 // src/main.nr:4:23
v17 = cast v13 as Field // src/main.nr:5:24
v18 = cast v16 as Field // src/main.nr:5:24
v19 = mul v17, v14 // src/main.nr:5:24
v21 = mul v18, Field -339696707650690765565390531789020390020 // src/main.nr:5:24
v22 = add v19, v21 // src/main.nr:5:24
enable_side_effects u1 1
v24 = cast v4 as Field // src/main.nr:4:23
v25 = cast v10 as Field // src/main.nr:4:23
v26 = mul v24, v9 // src/main.nr:4:23
v27 = mul v25, v22 // src/main.nr:4:23
v28 = add v26, v27 // src/main.nr:4:23
return v28
}I made |
…ir-lang/noir into af/unreachable-inst-under-side-effect
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs
Outdated
Show resolved
Hide resolved
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
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: 859e15a | Previous: e7a98f2 | Ratio |
|---|---|---|---|
rollup-base-private |
21.28 s |
16.54 s |
1.29 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
Some weeks ago I noticed that binary operations could be simplified, so for example if they always overflow we could replace them with a constrain failure. That was actually the first way I implemented that logic instead of eventually adding it to I'm going to try that in a separate PR just to see if it works, because I think it makes sense. |
|
In the end what I said above could be done but would require handling of |
Description
Problem*
Follow up for #9195 (comment)
Summary*
Adds new logic to
remove_unreachable_instructionsto handle always failing binary instructions which are under a predicate by entering anUnreachableUnderPredicatemode, in which instructions that have side effects get removed, and their results replaced by default values. This mode persists until the nextenable_side_effectsinstruction, or the end of the block.The reasoning to do so is that if the binary operation is known to fail, then until side effect conditions change, it doesn't matter if it was enabled or not, the subsequent operations cannot have an effect, so we can get rid of them. We keep their results, because within the same block those might be used, but we replace them with default values that may not even appear in the SSA as instructions, such as numeric constants.
Additional Context
Looking at the SSA of the program from the predecessor ticket, it changes as follows:
So the result is the same, but notice these changes:
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.