fix(ssa): Mislabeled instructions with side effects in EnableSideEffectsIf removal pass

[vezenovm]

Description

Problem*

Resolves #8236

For the given snippet in the issue we have the following SSA before constant folding:

Details

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v10 = make_array [u16 59890, u16 17417, u16 14409] : [u16; 3]
    v13 = call f1(v10, u1 1) -> [u1; 1]
    v15 = array_get v8, index u32 0 -> u16
    v16 = cast v15 as u32
    v18 = mod v16, u32 14409
    v19 = array_get v13, index u32 0 -> u1
    enable_side_effects v19
    v20 = array_get v9, index u32 0 -> u1
    v21 = call f1(v8, v20) -> [u1; 1]
    v22 = array_get v9, index u32 0 -> u1
    v23 = not v19
    enable_side_effects v23
    v24 = array_get v8, index u32 0 -> u16
    v25 = cast v24 as u32
    v27 = mod v25, u32 3
    v28 = lt v27, u32 3
    v29 = unchecked_mul v28, v23
    constrain v29 == v23, "Index out of bounds"
    v30 = array_get v8, index v27 -> u16
    v31 = cast v30 as u32
    v33 = lt u32 24993, v31
    enable_side_effects u1 1
    v34 = unchecked_mul v19, v22
    v35 = unchecked_mul v23, v33
    v36 = unchecked_add v34, v35
    return v36
}

After Constant Folding:

Details

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v10 = make_array [u16 59890, u16 17417, u16 14409] : [u16; 3]
    v13 = call f1(v10, u1 1) -> [u1; 1]
    v15 = array_get v8, index u32 0 -> u16
    v16 = cast v15 as u32
    v18 = mod v16, u32 14409
    v19 = array_get v13, index u32 0 -> u1
    enable_side_effects v19
    v20 = array_get v9, index u32 0 -> u1
    v21 = call f1(v8, v20) -> [u1; 1]
    v22 = not v19
    enable_side_effects v22
    v24 = mod v16, u32 3
    v25 = lt v24, u32 3
    v26 = unchecked_mul v25, v22
    constrain v26 == v22, "Index out of bounds"
    v27 = array_get v8, index v24 -> u16
    v28 = cast v27 as u32
    v30 = lt u32 24993, v28
    enable_side_effects u1 1
    v31 = unchecked_mul v19, v20
    v32 = unchecked_mul v22, v30
    v33 = unchecked_add v31, v32
    return v33
}

After EnableSideEffectsIf removal:

Details

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v10 = make_array [u16 59890, u16 17417, u16 14409] : [u16; 3]
    v13 = call f1(v10, u1 1) -> [u1; 1]
    v15 = array_get v8, index u32 0 -> u16
    v16 = cast v15 as u32
    v18 = mod v16, u32 14409
    v19 = array_get v13, index u32 0 -> u1
    enable_side_effects v19
    v20 = array_get v9, index u32 0 -> u1
    v21 = call f1(v8, v20) -> [u1; 1]
    v22 = not v19
    v24 = mod v16, u32 3
    v25 = lt v24, u32 3
    enable_side_effects v22
    v26 = unchecked_mul v25, v22
    constrain v26 == v22, "Index out of bounds"
    v27 = array_get v8, index v24 -> u16
    v28 = cast v27 as u32
    v30 = lt u32 24993, v28
    enable_side_effects u1 1
    v31 = unchecked_mul v19, v20
    v32 = unchecked_mul v22, v30
    v33 = unchecked_add v31, v32
    return v33
}

In the initial SSA, can see how v24 = array_get v8, index u32 0 -> u16 was deduplicated to a matching instruction that occurred under a different predicate. The result v24 is then used to compute the dynamic index of another array get. Those instructions calculating the index are being moved incorrectly causing an incorrect index to be computed.

Looking at the new SSA (After EnableSideEffectsIf removal) with the deduplicated array_get, we now use the actual result v15 from v15 = array_get v8, index u32 0 -> u16 when computing v16 = cast v15 as u32 -> v24 = mod v16, u32 3 -> v27 = array_get v8, index v24 -> u16.

After the EnableSideEffectsIf removal pass the modulo v24 = mod v16, u32 3 occurs under a different predicate from where it was previously. If we have a zero predicate our euclidean division returns zero for both the quotient and the remainder.

We have this final SSA with the enable side effects vars simplified:

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v11 = array_get v8, index u32 0 -> u16
    v12 = cast v11 as u32
    enable_side_effects u1 0
    v14 = array_get v9, index u32 0 -> u1
    v16 = call f1(v8, v14) -> [u1; 1]
    v18 = mod v12, u32 3
    v19 = lt v18, u32 3
    enable_side_effects u1 1
    constrain v19 == u1 1, "Index out of bounds"
    v21 = array_get v8, index v18 -> u16
    v22 = cast v21 as u32
    v24 = lt u32 24993, v22
    enable_side_effects u1 1
    return v24
}

We can see how the modulo is now under a false predicate when it would have been under a true predicate previously. Thus, we are getting a result of zero from the modulo operation instead of one as we would like, triggering an incorrect result.

Summary*

I simply switched to using Instruction::requires_acir_gen_predicate instead of responds_to_side_effects_var where Binary::requires_acir_gen_predicate is called internaly and BinaryOp::Div/BinaryOp::Mod are correctly labeled as always requiring ACIR gen predicates.

I could have went with just updating the responds_to_side_effects_var function inside of remove_enable_side_effects.rs, however, I felt it would be better to lean into Instruction::requires_acir_gen_predicate as they are attempting to do the same thing.
Comments above the call to responds_to_side_effects_var:

// If we hit an instruction which is affected by the side effects var then we must insert the
// `Instruction::EnableSideEffectsIf` before we insert this new instruction.

Comments above Instruction::requires_acir_gen_predicate:

/// If true the instruction will depend on `enable_side_effects` context during acir-gen.

We also had other mismatches between the methods on what instructions returned true for being affected by the side effects var. I felt it is better to be unified here and have alignment on which instructions depend on an ACIR predicate.

Disabling of the side effects var in ACIR gen is also based upon requires_acir_gen_predicate.

Additional Context

The snapshosts make the PR seem much bigger than it is in reality. The majority of the changes are contained within compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs.

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[github-actions]

Changes to circuit sizes

Generated at commit: 23a53fb8925fe5acd3cc8088291ad55d95e448f8, compared to commit: 94e839d82c13c90e7f5989e9ab6cea73ab2ba403

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
bench_eddsa_poseidon	-412 ✅	-2.28%	-412 ✅	-1.88%
to_bytes_integration	-206 ✅	-17.28%	-206 ✅	-15.20%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
regression	294 (+4)	+1.38%	3,836 (-2)	-0.05%
conditional_1	2,688 (-8)	-0.30%	8,046 (-8)	-0.10%
slices	668 (-4)	-0.60%	3,767 (-4)	-0.11%
hash_to_field	450 (-4)	-0.88%	3,485 (-4)	-0.11%
regression_7128	509 (-4)	-0.78%	3,355 (-4)	-0.12%
ram_blowup_regression	114,690 (-1,024)	-0.88%	377,161 (-1,024)	-0.27%
bench_eddsa_poseidon	17,683 (-412)	-2.28%	21,532 (-412)	-1.88%
to_bytes_integration	986 (-206)	-17.28%	1,149 (-206)	-15.20%

[TomAFrench]

I'd like to add some acirgen tests for this as it's not immediately clear why this is an issue (i.e. why does the predicate attached to the other array_get not handle this?)

[vezenovm]

(i.e. why does the predicate attached to the other array_get not handle this?)

EDIT: Oh I see what you mean. The index for v28 = array_get v8, index v25 -> u16 should be handling that predicate appropriately.

It looks like keeping the array get after the enable side effects var simply had the knockdown effect of keeping the modulo under the side effects variable as well. Otherwise, we risked the modulo being placed under a different predicate which is where the issue truly lies. Reference the main PR description for the full walkthrough of this bug.

[TomAFrench]

I found #8387 while reviewing this.

I'm supportive of consolidating onto requires_acir_gen_predicate if we find it fits our needs here although I'm quite leery of the number of instructions this change is affecting.

Ideally we should have tests which show that the new behaviour is desirable for each of these instructions.

[github-actions]

⚠️ 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: 3c50dab	Previous: 9dc9cf6	Ratio
rollup-merge	0.004 s	0.003 s	1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[jfecher]

LGTM. Fewer functions that do the same thing is good