fix(ssa): Do not hoist unsafe array_get in Brillig

[aakoshh]

Description

Problem*

Resolves #9804

Summary*

Reverts the changes in #9712 to return true for ArrayGet with an unsafe index value in requires_acir_gen_predicate and has_side_effects, regardless of the function runtime, to prevent the instruction from being separated from the constraints that prevent invalid indexes being used.

Additional Context

See the ticket comments for a rundown on the circumstances under which this was problematic. The TL;DR is that if we have a multi-dimensional array such as [[u8; 2]; 2] and we use an unsafe index to read the data like this:

fn main(i: u32) {
  ...
  let a = [[1,2], [3,4]];
  let b = a[i];
  let c = b[0];
  ...
}

then a[i] returns some invalid data in Brillig, which then fails when we try to use it in b[0], but since both array_get were considered pure, they got hoisted and Brillig executed them even if it should not have done so if it considered the control flow.

This affected two passes:

1. loop_invariant hoisted the instructions into the pre-header
2. constant_folding deduplicated the instructions if they appeared multiple times after unrolling

The 1st invalid array access (a[i]) causes the SSA interpreter to fail, and Brillig execution fails on the secondary array access (b[0]).

Alternative fix

Maybe it's true that in Brillig we should consider array_get free of side effects and require no predicate. In that case we could change can_be_hoisted to return false specifically for unsafe ArrayGet and use can_be_hoisted in constant_folding to decide whether it's safe to deduplicate, rather than has_side_effects. I'm not exactly sure which one of requires_acir_gen_predicate and has_side_effects we use under which circumstances.

Another question is whether we want the SSA interpreter to not fail if it executes an array_get with an invalid index when the runtime is Brillig, although I think the spirit of the comments suggests that there should always be a constraint to prevent this in Brillig.

I tried this in #9807

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]

⚠️ 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: 787adcf	Previous: 8cfbdb9	Ratio
rollup-root	0.005 s	0.004 s	1.25

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

CC: @TomAFrench

[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: 787adcf	Previous: 8cfbdb9	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	246 s	202 s	1.22

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

CC: @TomAFrench

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: a1dcad84562bb17c51974a1acdaab00797cf4846, compared to commit: 8cfbdb9d2cbe21faa883271d3259c726db275ad5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
slice_dynamic_index_inliner_min	+38 ❌	+1.80%
merkle_insert_inliner_max	+6 ❌	+1.12%
merkle_insert_inliner_min	+3 ❌	+0.80%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
slice_dynamic_index_inliner_min	2,154 (+38)	+1.80%
merkle_insert_inliner_max	543 (+6)	+1.12%
merkle_insert_inliner_min	377 (+3)	+0.80%
merkle_insert_inliner_zero	383 (+3)	+0.79%
nested_array_in_slice_inliner_min	806 (+4)	+0.50%
nested_array_in_slice_inliner_zero	806 (+4)	+0.50%
nested_array_in_slice_inliner_max	873 (+4)	+0.46%
simple_shield_inliner_max	676 (+3)	+0.45%
simple_shield_inliner_zero	676 (+3)	+0.45%
simple_shield_inliner_min	696 (+3)	+0.43%
nested_array_dynamic_inliner_min	1,334 (+3)	+0.23%
nested_array_dynamic_inliner_zero	1,334 (+3)	+0.23%
nested_array_dynamic_inliner_max	1,548 (+3)	+0.19%
slice_dynamic_index_inliner_max	1,690 (+3)	+0.18%
slice_dynamic_index_inliner_zero	1,690 (+3)	+0.18%

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: a1dcad84562bb17c51974a1acdaab00797cf4846, compared to commit: 8cfbdb9d2cbe21faa883271d3259c726db275ad5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
loop_invariant_regression_inliner_min	+324 ❌	+26.32%
slice_dynamic_index_inliner_min	+15 ❌	+0.33%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
loop_invariant_regression_inliner_min	1,555 (+324)	+26.32%
slice_dynamic_index_inliner_min	4,570 (+15)	+0.33%

[vezenovm]

LGTM, thanks for the ifnd. Seems it is not enough to say that an instruction is side effectual when it inserts more instructions. If it can potentially return an unexpected type, it is also side effectual.

[github-actions]

⚠️ 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: 9a9e46c	Previous: 8cfbdb9	Ratio
rollup-base-public	22.32 s	15.9 s	1.40

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

CC: @TomAFrench