fix(ssa): Start with checked operations in index calculations

[aakoshh]

Description

Problem*

Resolves #9856

Summary*

Implements @vezenovm 's idea from #9856 (comment)

When generating SSA for indexing arrays, use unchecked: false by default and let the simplify machinery promote it to unchecked: true if we know that it's safe. This prevents such SSA from being generated that 1) SSA interpreter wraps around, 2) ACIR overflows into u64 and beyond and 3) Brillig wraps around but it's protected by constraints.

This had some knock-on effects: now that we use mul instead of unchecked_mul to calculate an index, it can fail, and when we see that it will, remove_unreachable_indexes replaces the failing instruction with a default 0. It also does this if e.g. the add <prev-var>, u32 1 follows a failing mul: since the add requires a predicate, it is replaced by 0.

This has the unfortunate potential for causing type-errors with array_get where the array contains heterogenous data: if e.g. every index n * 4 + 3 contains a u1, but we replace that with index 0, that could be some different type. The result is somehow the SSA value that is assigned to the result will have the actual type from index 0, and then fail when it's used e.g. in a subsequent binary operation, as it doesn't match the other side.

I solved this by introducing a should_replace_instruction_with_defaults function, which verifies that a result = array_get arr, index 0 -> type checks out, ie. that typeof(result) == typeof(array)[0].

Another change I made to replace_unreachable_instructions was that it is now removing a failing binary instruction, leaving only the constraint. This is so that it's in line with the handling of array_get and slice operations. (This wasn't the full motivation, I wanted to remove and replace the binary with a default as well, but it turned out that doing so still did not allow me to detect where the result was used later on, because the simple_optimization already substitutes values, so what I get is just a value ID of the constant zero, not where it came from).

Additional Context

I added a test which expects that the SSA interpreter should fail on it, but there is no such integration test group. I'll create a separate issue for that: #9908

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: ebdbd21d05bb6f6b9081879249944eeda63e8409, compared to commit: dd83075be5758cf8c6a3a28e7c4a08a32fc9fde5

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
lambda_from_array	+8 ❌	+0.44%	+2,753 ❌	+215.92%
regression_9467	-4 ✅	-57.14%	-8 ✅	-29.63%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
lambda_from_array	1,809 (+8)	+0.44%	4,028 (+2,753)	+215.92%
nested_array_dynamic	3,599 (+367)	+11.36%	13,520 (+909)	+7.21%
hashmap	34,876 (+3,326)	+10.54%	98,656 (+4,706)	+5.01%
nested_array_in_slice	1,029 (+72)	+7.52%	5,599 (+166)	+3.06%
array_dynamic_nested_blackbox_input	172 (+42)	+32.31%	5,308 (+136)	+2.63%
regression_struct_array_conditional	76 (+10)	+15.15%	3,209 (+13)	+0.41%
regression_7612	19 (+4)	+26.67%	2,785 (+5)	+0.18%
lambda_from_global_array	26 (+2)	+8.33%	2,796 (+3)	+0.11%
regression_9467	3 (-4)	-57.14%	19 (-8)	-29.63%

[github-actions]

⚠️ 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: 591a433	Previous: dd83075	Ratio
private-kernel-reset	76858 opcodes	68870 opcodes	1.12

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

CC: @TomAFrench

[github-actions]

⚠️ 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: f631245	Previous: 13fbc6f	Ratio
rollup-base-private	1360 MB	506.72 MB	2.68

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

CC: @TomAFrench

[aakoshh]

Here's a minimised version of on of the failing tests:

fn main() {
    let b = unsafe { func_3() };
    if b {
        let c = unsafe { func_4() }[1532016929_u32].3;
        if c {
            let d = unsafe { func_3() };
        }
    }
}

unconstrained fn func_3() -> bool {
    true
}
unconstrained fn func_4() -> [(str<3>, str<3>, bool, bool); 4] {
    [
        ("ABC", "CDE", false, true),
        ("FGH", "IJK", false, true),
        ("LMN", "OPQ", false, true),
        ("RST", "UVW", false, true),
    ]
}

❯ cargo run -q -p nargo_cli -- compile --force --silence-warnings --show-ssa-pass "step 41" --show-ssa-pass "step 42"

After Checked to unchecked (1) (step 41):
acir(inline) predicate_pure fn main f0 {
  b0():
    v1 = call f1() -> u1
    enable_side_effects v1
    v3 = call f2() -> [([u8; 3], [u8; 3], u1, u1); 4]
    v6 = mul u32 1532016929, u32 4
    constrain u1 0 == v1, "attempt to multiply with overflow"
    v9 = array_get v3, index u32 0 -> u1
    v10 = unchecked_mul v1, v9
    enable_side_effects v10
    v11 = call f1() -> u1
    enable_side_effects u1 1
    return
}
brillig(inline) predicate_pure fn func_3 f1 {
  b0():
    return u1 1
}
brillig(inline) predicate_pure fn func_4 f2 {
  b0():
    v3 = make_array b"ABC"
    v6 = make_array b"CDE"
    v10 = make_array b"FGH"
    v14 = make_array b"IJK"
    v18 = make_array b"LMN"
    v22 = make_array b"OPQ"
    v26 = make_array b"RST"
    v30 = make_array b"UVW"
    v33 = make_array [v3, v6, u1 0, u1 1, v10, v14, u1 0, u1 1, v18, v22, u1 0, u1 1, v26, v30, u1 0, u1 1] : [([u8; 3], [u8; 3], u1, u1); 4]
    return v33
}

The application panicked (crashed).
Message:  Expected NumericType, found [u8; 3]
Location: compiler/noirc_evaluator/src/ssa/ir/types.rs:198

It seems to fail trying to simplify the unchecked_mul.

The problem seems to be that v9 = array_get v3, index u32 0 -> u1 is supposed to use v9 = array_get v3, index v6 -> u1, because at index u32 0 there certainly is a [u8; 3] (a.k.a. str<3>), not a u1.

Original failure came from

NOIR_AST_FUZZER_SEED=0x05de72c100031cc9 cargo test -q -p noir_ast_fuzzer_fuzz orig_vs_morph

[aakoshh]

Here's what goes down. Before remove_unreachable_instructions we have this:

...
    enable_side_effects v1
    v3 = call f2() -> [([u8; 3], [u8; 3], u1, u1); 4]
    v6 = mul u32 1532016929, u32 4
    v8 = add v6, u32 1
    v10 = add v6, u32 2
    v12 = add v6, u32 3
    v13 = array_get v3, index v12 -> u1
    v14 = unchecked_mul v1, v13
...

The remove_unreachable_instructions pass recognises that v6 will always fail and replaces it with a constraint. Then it goes into UnreachableUnderPredicate mode and replaces subsequent side-effecting instructions with their default, which in this case means v0, v10 and v12 all become 0.

When it comes to inspect array_get v3, index v12 it looks like array_get v3, index u32 0 because of having replaced v12 with 0, which is now a nonsensical instruction, but it looks safe and requires_acir_gen_predicate returns false, so the instruction is left in.

[github-actions]

⚠️ 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: 591a433	Previous: dd83075	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	1 s	3
test_report_zkpassport_noir_rsa_	2 s	1 s	2

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

CC: @TomAFrench

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: ebdbd21d05bb6f6b9081879249944eeda63e8409, compared to commit: dd83075be5758cf8c6a3a28e7c4a08a32fc9fde5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_9208_inliner_max	-1 ✅	-2.56%
regression_9208_inliner_min	-1 ✅	-2.56%
regression_9208_inliner_zero	-1 ✅	-2.56%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
regression_9208_inliner_max	38 (-1)	-2.56%
regression_9208_inliner_min	38 (-1)	-2.56%
regression_9208_inliner_zero	38 (-1)	-2.56%

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: ebdbd21d05bb6f6b9081879249944eeda63e8409, compared to commit: dd83075be5758cf8c6a3a28e7c4a08a32fc9fde5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_9546_inliner_max	-45 ✅	-41.67%
regression_9546_inliner_min	-45 ✅	-41.67%
regression_9546_inliner_zero	-45 ✅	-41.67%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
conditional_regression_short_circuit_inliner_min	220 (-3)	-1.35%
conditional_regression_short_circuit_inliner_max	202 (-3)	-1.46%
conditional_regression_short_circuit_inliner_zero	202 (-3)	-1.46%
regression_8235_inliner_max	35 (-3)	-7.89%
regression_8235_inliner_min	35 (-3)	-7.89%
regression_8235_inliner_zero	35 (-3)	-7.89%
regression_9208_inliner_max	48 (-5)	-9.43%
regression_9208_inliner_min	48 (-5)	-9.43%
regression_9208_inliner_zero	48 (-5)	-9.43%
regression_9594_inliner_max	40 (-5)	-11.11%
regression_9594_inliner_min	40 (-5)	-11.11%
regression_9594_inliner_zero	40 (-5)	-11.11%
regression_9546_inliner_max	63 (-45)	-41.67%
regression_9546_inliner_min	63 (-45)	-41.67%
regression_9546_inliner_zero	63 (-45)	-41.67%

[aakoshh]

Done. Here's the follow up ticket: #9930

[vezenovm]

Done. Here's the follow up ticket: #9930

Interesting, generalizing the check caused some failures. Looking at https://github.com/noir-lang/noir/actions/runs/17862409749/job/50796809412?pr=9888 these look to all be related to different inline types (e.g., calls ACIR/Brillig entry points and no predicates).

[aakoshh]

Okay, this is great, it gives me examples to demonstrate why it's not safe to replace everything with defaults when we go under UnreachableUnderPredicate.

One of the failures is regression_8236, where the interpreter changes its mind after remove_unreachable_instructions.

Here's is the SSA at various stages.

After flattening:

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v11 = call f1() -> [u1; 1]
    v13 = array_get v11, index u32 0 -> u1
    enable_side_effects v13
    v14 = call f1() -> [u1; 1]
    v15 = array_get v9, index u32 0 -> u1
    v16 = not v13
    enable_side_effects v16
    v17 = array_get v8, index u32 0 -> u16
    v18 = cast v17 as u32
    v20 = mod v18, u32 3
    v21 = array_get v8, index v20 -> u16
    v22 = cast v21 as u32
    v24 = lt u32 24993, v22
    enable_side_effects u1 1
    v26 = unchecked_mul v13, v15
    v27 = unchecked_mul v16, v24
    v28 = unchecked_add v26, v27
    return v28
}

After EnableSideEffectsIf removal:

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v11 = call f1() -> [u1; 1]
    v13 = array_get v11, index u32 0 -> u1
    enable_side_effects v13
    v14 = call f1() -> [u1; 1]
    v15 = array_get v9, index u32 0 -> u1
    v16 = not v13
    v17 = array_get v8, index u32 0 -> u16
    v18 = cast v17 as u32
    enable_side_effects v16
    v20 = mod v18, u32 3
    v21 = array_get v8, index v20 -> u16
    v22 = cast v21 as u32
    v24 = lt u32 24993, v22
    enable_side_effects u1 1
    v26 = unchecked_mul v13, v15
    v27 = unchecked_mul v16, v24
    v28 = unchecked_add v26, v27
    return v28
}

Notice how the enable_side_effects v16 sunk down: it jumped over v17 = array_get v8, index u32 0 -> u16 because that did not need a predicate because the index is safe, and now v17 is under enable_side_effects v13 instead.

After inlining brillig calls:

acir(inline) predicate_pure fn main f0 {
  b0(v8: [u16; 3], v9: [u1; 1]):
    v11 = make_array [u1 0] : [u1; 1]
    enable_side_effects u1 0
    v12 = make_array [u1 0] : [u1; 1]
    v14 = array_get v9, index u32 0 -> u1
    v15 = array_get v8, index u32 0 -> u16
    v16 = cast v15 as u32
    enable_side_effects u1 1
    v19 = mod v16, u32 3
    v20 = array_get v8, index v19 -> u16
    v21 = cast v20 as u32
    v23 = lt u32 24993, v21
    enable_side_effects u1 1
    return v23
}

Now we have constants for enable_side_effects, namely enable_side_effects v13 has been replaced by enable_side_effects u1 0.

At some point I added logic to remove_unreachable_instructions to say that if we are under enable_side_effects u1 0 then it means we cannot have side effects, so we can go into UnreachableUnderPredicate mode, which means we can replace any instruction which could fail with its default value. This helped me get rid of some leftover Load on branches that can never happen.

However if we expand this to replace all instructions, then it includes the v15 = array_get v8, index u32 0 -> u16 as well, which would not be considered to have a side effect as it is safe, and the types check out as well.

But if we remove that, then v15 is 0 instead of whatever was in the v0 array, and then v16 is zero, and then even though we have enable_side_effects u1 1, it doesn't give the expected result because v19 = mod v16, u32 3 now operates on 0.

So: something that should not have been under v13 bubbled up (or v16 sunk down), then v13 got disabled, v16 enabled, but that does not mean that the instructions which were under v13 were all meant to be disabled, only the ones that had potential side effects, which would not have migrated between side effect variables.

[vezenovm]

But if we remove that, then v15 is 0 instead of whatever was in the v0 array, and then v16 is zero, and then even though we have enable_side_effects u1 1, it doesn't give the expected result because v19 = mod v16, u32 3 now operates on 0.

So: something that should not have been under v13 bubbled up (or v16 sunk down), then v13 got disabled, v16 enabled, but that does not mean that the instructions which were under v13 were all meant to be disabled, only the ones that had potential side effects, which would not have migrated between side effect variables.

Could we not potentially run into the same issue even when gating for a zero index? Is it fine because we assume we do not have mismatched SSA types in our SSA input? I guess this is a fine assumption if we validate our SSA for this.

Makes sense, thank you for the detailed explanation. We handle something similar in ACIR gen. When we have an instruction which is not dependent on the predicate we use a predicate of one. This guarantees we get the value that was meant to be there rather than a default value

noir/compiler/noirc_evaluator/src/acir/mod.rs

Line 422 in 44b6be7

let predicate = if instruction.requires_acir_gen_predicate(dfg) {

I see we have the same fall back inside of should_replace_instruction_with_defaults.

[aakoshh]

Could we not potentially run into the same issue even when gating for a zero index?

I was thinking about this yesterday: when we see an ArrayGet with index 0, can we be sure that it returns the right data, considering that it could have migrated between side effect variables? I think the answer is yes, because if we had a non-empty array, then index 0 was safe to begin with and we had no reason to tamper with it, so there should not be any inconsistency in the types. If it turns out that we did have to set the index to a default value, but the types still check out, then it's okay to leave it as it is, because it will read data that fits the shape, but most likely the circuit will fail before this comes to pass.

Is it fine because we assume we do not have mismatched SSA types in our SSA input?

Yes, the assumption is that this type inconsistency can only be the product of this pass, and that it should be a temporary phenomenon that we must eliminate by the end of it.

One thing I wondered about was that the elimination is gated by whether we are in the UnreachableUnderPredicate mode, and effect variables stack up, but the reset mechanism treats them as independent. So, could we be in a situation like the following:

enable_side_effects v1
constraint v1 == 0                 // This activates the UnreachableUnderPreciate mode
v3 = <load-index-from-memory>      // Some side effecting operation, replaced by default, so v3 will be 0
enable_side_effects (v1*v2)        // If `v1` was off, this will be off, yet we are back to being `Reachable`
v4 = array_get v0, index v3 -> u1  // the data at 0 might not be a u1 any more, and we are not removing because we are Reachable

The reason I thought this will not pose a problem in practice, is that where we get in trouble is with composite types, such as [(T1, T2); 3], indexed as a[i].j, where the index comes size * i + j, and the problem is where j disappears, and one could think that we could have zeroed out the index under the influence of v1 and then executed the read under a different effect, but based on my experiments it looks like the index+offset operation never detaches from getting the array, since if the index was assigned separately to a variable, then the array would not be safely indexed, and it's actually the array-indexing plus tuple access operation together that triggers the index+offset code generation, so, as long as we use mul, the side effect variables will not jump over them.

[aakoshh]

When we have an instruction which is not dependent on the predicate we use a predicate of one.

Something similar to the one trick happens in simple_optimization as well.

[vezenovm]

Approving as to get the fix in but I want to follow-up with #9888 (comment) and Change the SSA interpreter handling of overflows