fix(ssa): Do not check ArrayGet/Set as unreachable for Brillig

[vezenovm]

Description

Problem*

Resolves a panic ( Assertion failed: Bit size for lhs 16 does not match op bit size 32) from a Noir sync that is currently blocking any more syncs.

Summary*

It looks like the Aztec end-to-end tests pass with the an ACIR runtime gate just on Instruction::ArrayGet/Instruction::ArraySet. I thought the core issue is most likely with the dummy array (thinking it was due to some mismatched types but I have not found the exact issue there yet). However, gating just the dummy array logic was not enough to fix the Brillig panic. Gating the entire array get/set check was the only thing that removed the panic.

However, we can do a hot fix here by only allowing this optimization for ACIR.

I only gated Instruction::ArrayGet and Instruction::ArraySet in case we still wanted to mark unreachable instructions for failing constrains in Brillig. Looking back at the history of when this pass was created it does look like it was mainly made to avoid ACIR compilation errors. So perhaps for the sake of simplicity it would be better to simply gate the entire pass to only be performed on ACIR.

Additional Context

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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: f8790f7	Previous: 9797518	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	4 s	3 s	1.33
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib	2 s	1 s	2

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

CC: @TomAFrench

[asterite]

Looks good!

I do wonder if the context.dfg.runtime().is_acir() check should be done depending on the side_effects_var value, but if all tests passes and there are no big regressions it probably means that gating it in all cases is probably fine.

[vezenovm]

I do wonder if the context.dfg.runtime().is_acir() check should be done depending on the side_effects_var value, but if all tests passes and there are no big regressions it probably means that gating it in all cases is probably fine.

Do you think it may be cleaner to just run this optimization on ACIR entirely? It looks like this pass was originally made for that runtime either way? I guess it does provide benefits for Brillig compilation where we potentially don't have to pass over many unreachable instructions.

[asterite]

Do you think it may be cleaner to just run this optimization on ACIR entirely? It looks like this pass was originally made for that runtime either way?

The pass was originally made to solve this issue: #8774

In there there's an always-failing constraint and later there's a load from a value that can't exist becase it's from an array_get from an empty array. There was an alternative fix at that time (#8943) but I thought it would be more correct to remove any instructions after an always-failing one and end the block with unreachable.

I don't know if that same code would fail in brillig, I'll check it now, but if not then we could gate it just to ACIR, though I feel that this is a good thing to do in general as it removes redundant instructions.

[vezenovm]

though I feel that this is a good thing to do in general as it removes redundant instructions.

Yes I agree. I'll try to keep what we can for Brillig then.

EDIT: We no longer get the Aztec end to end tests failure when gating just ArrayGet/Set