fix(ssa): Put some default in Value::uninitialized for references in the SSA interpreter

[aakoshh]

Description

Problem*

Resolves #9594

Summary*

1. Changes Value::uninitialized in the SSA interpreter to store Some default value for Reference types. This way a subsequent Load will not fail, which it currently does even if side effects are disabled, if the value has never been stored to.
2. Run an extra dead_instruction_elimination after the first mem2reg pass to get rid of any Load that isn't actually needed. This isn't needed to fix the problem, but it was left in because it has some positive effects on the bytecode size.
3. Fix remove_unreachable_instructions to insert a store after an allocate in zeroed_value when replacing an instruction that returned a reference. This way a Load in the interpreter won't fail if executed on the resulting SSA.

Additional Context

In the integration test we have this code:

fn main(a: bool, mut b: bool, c: i32) -> pub i32 {
    if a {
        let mut e: [&mut bool] = &[];
        b = *e[(1132210792_u32 % e.len())];
        c
    } else {
        2
    }
}

e is an empty slice, so indexing it with anything should fail. However a will be false, so we should not hit this problem.

The SSA before the first mem2reg reflects the state where a constraint exists on the index, but it's not an always-fail one, and we never hit that block during evaluation:

After Inlining (step 7):
acir(inline) fn main f0 {
  b0(v0: u1, v1: u1, v2: i32):
    v4 = allocate -> &mut u1
    store v1 at v4
    jmpif v0 then: b1, else: b2
  b1():
    v6 = make_array [] : [&mut u1]
    v7 = allocate -> &mut u32
    store u32 0 at v7
    v9 = allocate -> &mut [&mut u1]
    store v6 at v9
    v10 = load v7 -> u32                          	// src/main.nr:4:17
    v12 = mod u32 1132210792, v10                 	// src/main.nr:4:17
    v13 = load v7 -> u32                          	// src/main.nr:4:17
    v14 = load v9 -> [&mut u1]                    	// src/main.nr:4:17
    v15 = lt v12, v13                             	// src/main.nr:4:17
    constrain v15 == u1 1, "Index out of bounds"  	// src/main.nr:4:17
    v17 = array_get v14, index v12 -> &mut u1     	// src/main.nr:4:14
    v18 = load v17 -> u1                          	// src/main.nr:4:14
    store v18 at v4                               	// src/main.nr:4:14
    jmp b3(v2)
  b2():
    jmp b3(i32 2)
  b3(v3: i32):
    return v3
}

--- Interpreter result after Inlining (step 7):
Ok(i32 2)
---

Note that v17 gets from the empty slice, v18 loads the value of the reference, and then it's stored at v4.

After mem2reg the constraint changes into an always-fail one, however this has no effect on the rest of the instructions in the block. mem2reg also removes the store v18 at v4, but it cannot remove the v18 = load v17 instruction.

After Mem2Reg (step 8):
acir(inline) fn main f0 {
  b0(v0: u1, v1: u1, v2: i32):
    v4 = allocate -> &mut u1
    jmpif v0 then: b1, else: b2
  b1():
    v6 = make_array [] : [&mut u1]
    v7 = allocate -> &mut u32
    v8 = allocate -> &mut [&mut u1]
    v11 = mod u32 1132210792, u32 0               	// src/main.nr:4:17
    constrain u1 0 == u1 1, "Index out of bounds" 	// src/main.nr:4:17
    v14 = array_get v6, index v11 -> &mut u1      	// src/main.nr:4:14
    v15 = load v14 -> u1                          	// src/main.nr:4:14
    jmp b3(v2)
  b2():
    jmp b3(i32 2)
  b3(v3: i32):
    return v3
}

--- Interpreter result after Mem2Reg (step 8):
Ok(i32 2)

When we reach the flattening step, an enable_side_effects v0 appears before the array_get:

After Flattening (1) (step 19):
acir(inline) predicate_pure fn main f0 {
  b0(v0: u1, v1: u1, v2: i32):
    v3 = allocate -> &mut u1
    enable_side_effects v0
    v4 = make_array [] : [&mut u1]
    v5 = allocate -> &mut u32
    v6 = allocate -> &mut [&mut u1]
    v9 = mod u32 1132210792, u32 0                	// src/main.nr:4:17
    constrain u1 0 == v0, "Index out of bounds"   	// src/main.nr:4:17
    v11 = array_get v4, index v9 -> &mut u1       	// src/main.nr:4:14
    v12 = load v11 -> u1                          	// src/main.nr:4:14
    v13 = not v0
    enable_side_effects u1 1
    v15 = cast v0 as i32
    v16 = cast v13 as i32
    v17 = unchecked_mul v15, v2
    v19 = unchecked_mul v16, i32 2
    v20 = unchecked_add v17, v19
    return v20
}

With the changes to the SSA Interpreter, this now returns a 0 value in the reference, and load v11 can succeed.

DIE

Eventually the DIE pass removes this as it's not used. My observation was even though mem2reg could not remove the load, since it removed the store, if we run DIE now, it's also eliminated at an early stage (before flattening), when we are still in a position of evaluating only the blocks which are activated by the jumps.

After the extra DIE pass, we have the following state:

After Dead Instruction Elimination (step 9):
acir(inline) fn main f0 {
  b0(v0: u1, v1: u1, v2: i32):
    jmpif v0 then: b1, else: b2
  b1():
    v7 = mod u32 1132210792, u32 0                	// src/main.nr:4:17
    constrain u1 0 == u1 1, "Index out of bounds" 	// src/main.nr:4:17
    jmp b3(v2)
  b2():
    jmp b3(i32 2)
  b3(v3: i32):
    return v3
}

--- Interpreter result after Dead Instruction Elimination (step 9):
Ok(i32 2)

This doesn't handle the case of dangling loads for values which are used; for those we still get a compilation error. It only brings forward the "healing" of the SSA by the DIE, so we don't get a period of SSA states which cannot be interpreted.

Remove Unreachable Instructions

If we disable the new DIE pass, then the Remove Unreachable Instructions pass handles the case in the following way:

After Remove Unreachable Instructions (1) (step 33):
acir(inline) predicate_pure fn main f0 {
  b0(v0: u1, v1: u1, v2: i32):
    v3 = allocate -> &mut u1
    v4 = make_array [] : [&mut u1]
    v5 = allocate -> &mut u32
    v6 = allocate -> &mut [&mut u1]
    enable_side_effects v0
    v9 = mod u32 1132210792, u32 0                	// src/main.nr:4:17
    constrain u1 0 == v0, "attempt to calculate the remainder with a divisor of zero"	// src/main.nr:4:17
    constrain u1 0 == v0, "Index out of bounds"   	// src/main.nr:4:17
    v11 = allocate -> &mut u1
    store u1 0 at v11
    v12 = load v11 -> u1                          	// src/main.nr:4:14
    enable_side_effects u1 1
    v14 = cast v0 as i32
    v15 = unchecked_mul v14, v2
    v17 = unchecked_add v15, i32 2
    return v17
}

Notice that v11 = allocate -> &mut u1; store u1 0 at v11 replaced the array_get because of the conditional-fail constraints preceding it, and together the allows load v11 to be interpreted.

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 number of Brillig opcodes executed

Generated at commit: fe973e05e778e64b110f772f3cd7740ba61081bd, compared to commit: 5657704f1688e5d00cbf5cb2133b5f2f75eb34bd

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
hashmap_inliner_zero	-1,633 ✅	-2.25%
numeric_type_alias_inliner_max	-103 ✅	-7.24%
numeric_type_alias_inliner_zero	-103 ✅	-7.24%
derive_inliner_zero	-31 ✅	-11.27%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
slice_regex_inliner_zero	4,032 (-3)	-0.07%
uhashmap_inliner_max	143,571 (-113)	-0.08%
slices_inliner_max	2,488 (-3)	-0.12%
strings_inliner_min	1,567 (-3)	-0.19%
strings_inliner_zero	1,553 (-3)	-0.19%
strings_inliner_max	1,509 (-3)	-0.20%
uhashmap_inliner_min	173,826 (-379)	-0.22%
hashmap_inliner_max	56,953 (-174)	-0.30%
hashmap_inliner_min	79,316 (-271)	-0.34%
brillig_pedersen_inliner_max	585 (-3)	-0.51%
pedersen_check_inliner_max	585 (-3)	-0.51%
brillig_pedersen_inliner_min	585 (-3)	-0.51%
pedersen_check_inliner_min	585 (-3)	-0.51%
brillig_pedersen_inliner_zero	585 (-3)	-0.51%
pedersen_check_inliner_zero	585 (-3)	-0.51%
slices_inliner_zero	2,729 (-15)	-0.55%
array_to_slice_inliner_max	953 (-6)	-0.63%
slices_inliner_min	3,750 (-57)	-1.50%
uhashmap_inliner_zero	163,784 (-2,532)	-1.52%
hashmap_inliner_zero	71,079 (-1,633)	-2.25%
numeric_type_alias_inliner_max	1,319 (-103)	-7.24%
numeric_type_alias_inliner_zero	1,319 (-103)	-7.24%
derive_inliner_zero	244 (-31)	-11.27%

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: fe973e05e778e64b110f772f3cd7740ba61081bd, compared to commit: 5657704f1688e5d00cbf5cb2133b5f2f75eb34bd

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
slices_inliner_zero	-15 ✅	-0.91%
array_to_slice_inliner_max	-6 ✅	-1.20%
slices_inliner_min	-46 ✅	-2.11%
derive_inliner_zero	-13 ✅	-4.69%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
hashmap_inliner_zero	7,862 (+27)	+0.34%
uhashmap_inliner_zero	6,740 (+4)	+0.06%
uhashmap_inliner_min	7,144 (-6)	-0.08%
hashmap_inliner_min	8,864 (-9)	-0.10%
slices_inliner_max	1,695 (-3)	-0.18%
slice_regex_inliner_zero	1,637 (-3)	-0.18%
hashmap_inliner_max	18,679 (-58)	-0.31%
strings_inliner_max	902 (-3)	-0.33%
strings_inliner_min	894 (-3)	-0.33%
strings_inliner_zero	884 (-3)	-0.34%
uhashmap_inliner_max	11,194 (-39)	-0.35%
pedersen_check_inliner_max	403 (-3)	-0.74%
brillig_pedersen_inliner_max	403 (-3)	-0.74%
pedersen_check_inliner_min	403 (-3)	-0.74%
brillig_pedersen_inliner_min	403 (-3)	-0.74%
brillig_pedersen_inliner_zero	403 (-3)	-0.74%
pedersen_check_inliner_zero	403 (-3)	-0.74%
numeric_type_alias_inliner_max	333 (-3)	-0.89%
numeric_type_alias_inliner_zero	333 (-3)	-0.89%
slices_inliner_zero	1,635 (-15)	-0.91%
array_to_slice_inliner_max	495 (-6)	-1.20%
slices_inliner_min	2,134 (-46)	-2.11%
derive_inliner_zero	264 (-13)	-4.69%

[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: 52a64ff	Previous: 3629a25	Ratio
test_report_zkpassport_noir_rsa_	2 s	1 s	2

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

CC: @TomAFrench

[jfecher]

The SSA produced by mem2reg is technically valid so we shouldn't have to fix it after the fact. In general, we shouldn't rely on other SSA passes to fix corner cases in previous passes either since this makes them more brittle.

I think the actual issue here is that the interpreter errors at all for this code. It errors because side effects are disabled for that branch in flattening and we return Value::uninitialized for a reference type which currently does not give a default value to the resulting reference. We should change Value::uninitialized to recur in the case of references to store an uninitialized reference element to that reference as well. Then the code should run.

That being said, there are some brillig improvements to adding this pass so I'd be okay with merging it once the other change is made so we can confirm it works without the pass and if we are okay with the tradeoff of a bit more compilation time for these brillig improvements.

[aakoshh]

@jfecher I changed Value::uninitialized to create reference types with Some default element instead of None. I see the interpreter uses Value::uninitialized in interpret_array_get when side effects are disabled.

However, remove_unreachable_instructions replaces instructions with side effects with default values after always-fail instructions. This was something I added in #9462 to get around a similar situation.

In the case of this integration test, similar to the description of that PR, we will have the array_get replaced with allocate, and in the case of the interpreter allocate doesn't use uninitialized, so it will still fail on load:

After Remove Unreachable Instructions (step 33):
acir(inline) predicate_pure fn main f0 {
  b0(v0: u1, v1: u1, v2: i32):
    v3 = allocate -> &mut u1
    v4 = make_array [] : [&mut u1]
    v5 = allocate -> &mut u32
    v6 = allocate -> &mut [&mut u1]
    enable_side_effects v0
    v9 = mod u32 1132210792, u32 0                	// src/main.nr:4:17
    constrain u1 0 == v0, "attempt to calculate the remainder with a divisor of zero"	// src/main.nr:4:17
    constrain u1 0 == v0, "Index out of bounds"   	// src/main.nr:4:17
    v11 = allocate -> &mut u1
    v12 = load v11 -> u1                          	// src/main.nr:4:14
    enable_side_effects u1 1
    v14 = cast v0 as i32
    v15 = unchecked_mul v14, v2
    v17 = unchecked_add v15, i32 2
    return v17
}

load failing when store is missing was a great way to detect problems with mem2reg. Maybe we can return default from interpret_load or insert a default in interpret_allocate, when side effects are disabled?

Although technically that would be wrong, since requires_acir_gen_predicate returns false for Load and Allocate as well 😕

[jfecher]

@aakoshh it sounds like remove_unreachable_instructions produces invalid SSA then. Regardless of the value for enable_side_effects, allocate instructions are still always expected to be stored to

[aakoshh]

Okay, I'll have a look at undoing that change then.

[github-actions]

ACVM Benchmarks

Details

Benchmark suite	Current: 61f614f	Previous: 3629a25	Ratio
purely_sequential_opcodes	248426 ns/iter (± 976)	249317 ns/iter (± 1414)	1.00
perfectly_parallel_opcodes	217858 ns/iter (± 2016)	218311 ns/iter (± 1895)	1.00
perfectly_parallel_batch_inversion_opcodes	2780914 ns/iter (± 10320)	2776515 ns/iter (± 30609)	1.00

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

[jfecher]

@aakoshh I'm not sure if it needs to be undone entirely - maybe it could be changed to also store a default value to the element?

[aakoshh]

Yeah that's what I was thinking as well: insert a store after the allocate. Reverting would not have solved the problem that array_get on an empty array fails even with side effects disabled.

[github-actions]

Execution Time

Details

Benchmark suite	Current: 61f614f	Previous: 5657704	Ratio
private-kernel-inner	0.015 s	0.014 s	1.07
private-kernel-reset	0.153 s	0.154 s	0.99
private-kernel-tail	0.01 s	0.01 s	1
rollup-base-private	0.267 s	0.265 s	1.01
rollup-base-public	0.16 s	0.171 s	0.94
rollup-block-root	13.2 s	13.4 s	0.99
rollup-merge	0.002 s	0.002 s	1
rollup-root	0.004 s	0.004 s	1
semaphore-depth-10	0.019 s	0.019 s	1
sha512-100-bytes	0.102 s	0.098 s	1.04

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

[github-actions]

Opcode count

Details

Benchmark suite	Current: 61f614f	Previous: 5657704	Ratio
private-kernel-inner	14792 opcodes	14792 opcodes	1
private-kernel-reset	68868 opcodes	68868 opcodes	1
private-kernel-tail	11177 opcodes	11177 opcodes	1
rollup-base-private	221335 opcodes	221335 opcodes	1
rollup-base-public	159954 opcodes	159954 opcodes	1
rollup-block-root-empty	68106 opcodes	68106 opcodes	1
rollup-block-root-single-tx	964509 opcodes	964513 opcodes	1.00
rollup-block-root	965795 opcodes	965799 opcodes	1.00
rollup-merge	1409 opcodes	1409 opcodes	1
rollup-root	2631 opcodes	2631 opcodes	1
semaphore-depth-10	5700 opcodes	5700 opcodes	1
sha512-100-bytes	13173 opcodes	13173 opcodes	1

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

[github-actions]

Compilation Time

Details

Benchmark suite	Current: 61f614f	Previous: 5657704	Ratio
private-kernel-inner	1.716 s	1.728 s	0.99
private-kernel-reset	7.746 s	7.996 s	0.97
private-kernel-tail	1.456 s	1.328 s	1.10
rollup-base-private	16.54 s	15.58 s	1.06
rollup-base-public	13.1 s	13.74 s	0.95
rollup-block-root-empty	19.78 s	20.82 s	0.95
rollup-block-root-single-tx	188 s	187 s	1.01
rollup-block-root	182 s	191 s	0.95
rollup-merge	1.322 s	1.358 s	0.97
rollup-root	1.584 s	1.438 s	1.10
semaphore-depth-10	0.813 s	0.781 s	1.04
sha512-100-bytes	1.582 s	1.601 s	0.99

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

[github-actions]

Artifact Size

Details

Benchmark suite	Current: 61f614f	Previous: 5657704	Ratio
private-kernel-inner	708.9 KB	708.9 KB	1
private-kernel-reset	2032.2 KB	2032.5 KB	1.00
private-kernel-tail	535.2 KB	535.2 KB	1
rollup-base-private	4319.6 KB	4319.6 KB	1
rollup-base-public	3332 KB	3332 KB	1
rollup-block-root-empty	3846.6 KB	3847.1 KB	1.00
rollup-block-root-single-tx	30744.8 KB	30747.8 KB	1.00
rollup-block-root	30775.7 KB	30780.5 KB	1.00
rollup-merge	187 KB	187 KB	1
rollup-root	388.8 KB	388.9 KB	1.00
semaphore-depth-10	631.5 KB	631.5 KB	1
sha512-100-bytes	525.5 KB	525.5 KB	1

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

[github-actions]

Test Suite Duration

Details

Benchmark suite	Current: 61f614f	Previous: 5657704	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	97 s	97 s	1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts	108 s	108 s	1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	158 s	150 s	1.05
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib	205 s	207 s	0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib	32 s	32 s	1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib	552 s
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	102 s	102 s	1
test_report_noir-lang_noir-bignum_	232 s	467 s	0.50
test_report_noir-lang_noir_bigcurve_	292 s	310 s	0.94
test_report_noir-lang_sha256_	15 s	14 s	1.07
test_report_noir-lang_sha512_	14 s	13 s	1.08
test_report_zkpassport_noir-ecdsa_	2 s	2 s	1
test_report_zkpassport_noir_rsa_	2 s	2 s	1

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

[github-actions]

Execution Memory

Details

Benchmark suite	Current: 61f614f	Previous: 3629a25	Ratio
private-kernel-inner	206.72 MB	206.72 MB	1
private-kernel-reset	243.53 MB	243.53 MB	1
private-kernel-tail	195.94 MB	195.94 MB	1
rollup-base-private	500.25 MB	500.25 MB	1
rollup-base-public	432.9 MB	432.9 MB	1
rollup-block-root	1500 MB	1500 MB	1
rollup-merge	328.19 MB	328.19 MB	1
rollup-root	330.55 MB	330.55 MB	1
semaphore_depth_10	69.5 MB	69.5 MB	1
sha512_100_bytes	54.99 MB	54.99 MB	1

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

[github-actions]

Compilation Memory

Details

Benchmark suite	Current: 61f614f	Previous: 3629a25	Ratio
private-kernel-inner	239.63 MB	239.64 MB	1.00
private-kernel-reset	549.09 MB	549.1 MB	1.00
private-kernel-tail	213.85 MB	213.85 MB	1
rollup-base-private	1350 MB	1350 MB	1
rollup-base-public	1400 MB	1400 MB	1
rollup-block-root-empty	1010 MB	1090 MB	0.93
rollup-block-root-single-tx	9520 MB	9580 MB	0.99
rollup-block-root	9520 MB	9590 MB	0.99
rollup-merge	330.56 MB	330.56 MB	1
rollup-root	341.25 MB	341.34 MB	1.00
semaphore_depth_10	104.75 MB	104.76 MB	1.00
sha512_100_bytes	233.05 MB	233.16 MB	1.00

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