chore: add regression to ensure array_get optimization is valid

[guipublic]

Description

Problem

Resolves https://ai.cantina.xyz/share/M8ptl1OQrTupbd8JiV0HrY-_yeqbrn-xa4Doh99LGB0/view?scanId=d3ca5ea4-1e0e-4943-8a82-11523cacab30&findingId=70fed138-29b7-41b5-89c2-5926714b4508

Summary

The reported issue mentions that try_optimize_array_get_from_previous_set() should not optimise array_get with a conditional array_set.
This is correct, and it is what happens currently.
So I did not change the implementation but rather added a test that check that it works explicitly.

Additional Context

The reason the issue was reported is because try_optimize_array_get_from_previous_set() does not check for enable_side_effect at all. Instead it relies on the fact that accessing arrays from a conditional branch will never lead to an array-set, but rather:

• a Load instruction, or
• an IfElse instruction, or
• an arithmetic operation (when IfElse are removed).

Since there is no explicit check, this statement may become incorrect in the future (due to changes in the code), so I added an integration test to validate explicitely that reading an array after a conditional array_set works.

User Documentation

Check one:

• No user documentation needed.
• Changes in docs/ included in this PR.
• [For Experimental Features] Changes in docs/ 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.

[TomAFrench]

Can we have an SSA level test for this?

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 084587b	Previous: a9bcd7f	Ratio
private-kernel-reset	0.107 s	0.086 s	1.24

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: 084587b	Previous: a9bcd7f	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	2 s	1.50

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

CC: @TomAFrench

[guipublic]

Can we have an SSA level test for this?

Not really, I think it would be better to add a SSA validation check that ensures there is no 'array-get of an array-set under a conditional'

[TomAFrench]

Yep, that sounds good.

[asterite]

Hmm... the test program doesn't test that ArrayGet isn't optimized based on an ArraySet with a different predicate... because the ArrayGet is done through a load, so it never reaches the ArraySet in the first place.

I think it would be better to add a SSA validation check that ensures there is no 'array-get of an array-set under a conditional'

The problem with this is that while an SSA is being built, ArrayGet might be optimized out because of this simplify rule. Then when we validate the SSA, those ArrayGet will be gone... but maybe they got simplified with an incorrect assumption.

[asterite]

Not really, I think it would be better to add a SSA validation check that ensures there is no 'array-get of an array-set under a conditional'

Actually, this SSA validation would be enough, I think. The only place where it would become invalid is when ValueMerger operates, but in that case it's safe to do that (assuming the SSA is valid at that point). So ignore my previous comments :-)

[asterite]

On second (third?) thought, the SSA validation might not be enough, as I mentioned before. Imagine an SSA optimization pass that inserts array_get. Because simplify will check past array_set, and because that array_get didn't exist before this SSA optimization ran, the instruction might get simplified. Then when SSA validation runs the array_get won't be there anymore. Well, this is what ValueMerger does, but I think it does it safely. But I don't know if any other SSA optimizations rearrange things around or insert array_get in a way that they could get incorrectly simplified.

[TomAFrench]

But I don't know if any other SSA optimizations rearrange things around or insert array_get in a way that they could get incorrectly simplified.

I think we should just pull this out of the instruction insertion logic and have an SSA pass devoted to this. I don't think this optimization really triggers often and we can then reason a lot more strongly about safety in a pass.