fix(ssa): Use can_be_hoisted in constant_folding

[aakoshh]

Description

Problem*

Alternative fix to #9805

Summary*

Experiment with keeping #9712 which considers ArrayGet in Brillig to return false from requires_acir_gen_predicate and has_side_effects, and instead change constant_folding to base its decision on loop_invariant::can_be_hoisted to decide whether to deduplicate instructions into the common denominator, instead of has_side_effects.

Change can_be_hoisted to have special handling for ArrayGet where we just look whether the index is safe, ignoring the runtime.

This required using has_side_effects from can_be_hoisted for Binary instructions, to pass the unit tests in constant_folding; has_side_effects has some extra sophistication around constants in certain operations (see below).

Additional Context

has_side_effect and requires_acir_gen_predicate are not equivalent for Binary:

• has_side_effects:
  • considers unchecked arithmetic to have side effects unless they operate on Field
  • inspects whether the number of bits is known in shl and shr
  • inspects whether the denominator in div and mod are known constants
• requires_acir_gen_predicate:
  • considers unchecked arithmetic to require predicate for unsigned only
  • does not inpect div, mod, shl and shr for known constants.

Could these be reconciled?

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: 8b0273a188d4129bb626295d9b33c22d81d89893, compared to commit: 7e7cb4c5e7542528b8d73ad6ecca19bb2f1ee697

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
regression_5252_inliner_zero	-40 ✅	-0.00%
loop_break_regression_8319_inliner_min	-82 ✅	-12.17%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
poseidon_bn254_hash_width_3_inliner_min	168,361 (-6)	-0.00%
poseidonsponge_x5_254_inliner_min	185,131 (-8)	-0.00%
regression_5252_inliner_min	923,995 (-40)	-0.00%
poseidonsponge_x5_254_inliner_zero	183,106 (-8)	-0.00%
regression_5252_inliner_zero	911,504 (-40)	-0.00%
loop_break_regression_8319_inliner_min	592 (-82)	-12.17%

[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: 806233c	Previous: 7e7cb4c	Ratio
test_report_noir-lang_sha512_	15 s	12 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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: f4b0637	Previous: f25ebd6	Ratio
rollup-root	0.005 s	0.004 s	1.25

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

CC: @TomAFrench

[aakoshh]

It looks like using has_side_effects instead of requires_acir_gen_predicate for Binary in can_be_hoisted causes some big size regressions. Perhaps because has_side_effects returns false for Cast, whereas can_be_hoisted only returns true if it can prove it's safe, so some Casts won't be deduplicated in constant folding.

Is it worth trying to bring some of the logic to inspect constants into requires_acir_gen_predicate, even if we don't adopt this approach of considering ArrayGet pure in Brillig?

[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: 0addd06	Previous: f25ebd6	Ratio
rollup-merge	1.84 s	1.354 s	1.36

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

CC: @TomAFrench

[aakoshh]

It looks like in lambda_from_array the difference in the bytecode size came from not being able to hoist make_array instructions into the common dominator, because in Brillig we considered it unsafe to be hoisted, and the can_hoist_invariant method had an override for it.

Changed it so that can_be_hoisted returns WithRefCount for this in Brillig, which now constant_folding interprets as true.

[vezenovm]

Is it worth trying to bring some of the logic to inspect constants into requires_acir_gen_predicate, even if we don't adopt this approach of considering ArrayGet pure in Brillig?

We would have to look at how the instructions are implemented in ACIR. For certain operations we insert values based upon the predicate even if they would not fail. For example we use the current predicate for div even if we know the operation is technically a "safe" div.

[vezenovm]

I like this additional enum variant instead of the early return on MakeArray before can_be_hoisted. However, I'm not sure if the rest of the PR is necessary with #9805 now being merged. It makes sense to me now to say that an OOB Brillig array get does in fact have side effects as it can potentially return an unexpected type.

[aakoshh]

Okay, then I’ll undo those and keep this. Do you see value in trying to use can_be _hoisted from constant_folding, or was that good based on has_side_effects?

[vezenovm]

I think it is good based off of has_side_effects. I think what they are trying to tell us is different. Deduplicating vs. hoisting a single instruction can lead to different semantics. For example, Cast does not have side effects on its own and can be deduplicated if we find a matching Cast. However, it may depend on a range check and cannot be hoisted when in isolation.

[aakoshh]

Will the deduplication across sibling blocks in the the common dominator not detach the Cast from its RangeCheck though?

[vezenovm]

As it is a deduplication we can assume that the other cast has an appropriate range check.

[aakoshh]

Right, so in our case it looks like as long as a truncate was emitted, the cast will stay safe, because with the logic based on has_side_effects both can be hoisted. can_be_hoisted hoists all truncate, but not all cast, so it's even more conservative.

Furthermore, not sure if it's by design or coincidence, but constant folding happens after the flattening of the CFG in ACIR, so nothing is hoisted into any common ancestor, as there is only one block.

[vezenovm]

I'm realizing we also already hoist safe div/mod instructions when we know the induction variable is the divisor and will never be zero. So it is most likely fine to use has_side_effects inside of can_be_hoisted over requires_acir_gen_predicate.

[aakoshh]

How about with something similar to the do_not_hoist_control_dependent_cast test in loop_invariant?

@vezenovm that's exactly what I used as a starter for #9807 (comment)

As I demonstrated in #9807 (comment) constant folding, when applied a number of times, will hoist the cast into the common ancestor (based on has_side_effects), but then I couldn't come up with anything to exploit this and turn it into a runtime failure, because in practice it was protected by the truncate which got hoisted with it. I used Brillig to even have a block to hoist into, and tried the SSA CLI to remove the truncate, but the interpreter did not die even then. I also tried shl <casted-i8>, 7 but that just returns 0 even on an overflowed i8 🤷 Anything that would fail doesn't get hoisted.

[vezenovm]

Furthermore, not sure if it's by design or coincidence, but constant folding happens after the flattening of the CFG in ACIR, so nothing is hoisted into any common ancestor, as there is only one block.

The side effects var is only used during fold_constants_using_constraints so I do believe in this case it was intentional. After adding dominance based deduplication though I think this should be fine to place before flattening. However, I don't know if we would desire that as constant folding stores a lot of stuff in memory and performing it early in the pipeline may not lead to desired results.

[vezenovm]

@vezenovm that's exactly what I used as a starter for #9807 (comment)

Ah sorry I was reading too quickly and skimmed over the actual Noir snippet.

because in practice it was protected by the truncate which got hoisted with it.

I guess this is where the difference between hoisting in isolation vs. deduplication came into play. As we only hoist into the common denominator if the instructions are matching we don't end up making a safe cast unsafe as its preceding instructions are expected to also be hoisted. This does still feel slightly brittle but it seems to be fine based off your testing.

As I see no regressions I would be good with moving to using can_be_hoisted in constant_folding but curious if @TomAFrench has an opinion here. The main reason I would advocate for still just using Instruction::has_side_effects would be to maintain the simplicity of constant_folding.