fix: don't deduplicate binary math of unsigned types

[asterite]

Description

Problem

Resolves #6834

Summary

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]

Changes to Brillig bytecode sizes

Generated at commit: 820178286e8332a05e4dfe5f697847ebaf40264e, compared to commit: 0d7642cb2071fbee148978a89a0922bfffe5be6a

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
sha2_byte	+8 ❌	+0.29%
poseidonsponge_x5_254	+10 ❌	+0.24%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
sha2_byte	2,775 (+8)	+0.29%
poseidonsponge_x5_254	4,264 (+10)	+0.24%
regression_5252	4,604 (+10)	+0.22%
poseidon_bn254_hash	5,435 (+10)	+0.18%
poseidon_bn254_hash_width_3	5,435 (+10)	+0.18%
uhashmap	14,019 (+15)	+0.11%
hashmap	21,813 (+12)	+0.06%

[github-actions]

Peak Memory Sample

Program	Peak Memory
keccak256	78.15M
workspace	122.62M
regression_4709	423.26M
ram_blowup_regression	1.58G
private-kernel-tail	206.71M
private-kernel-reset	720.29M
private-kernel-inner	291.91M
parity-root	171.71M

[github-actions]

Compilation Sample

Program	Compilation Time	%
sha256_regression	0m1.369s	1%
regression_4709	0m0.822s	6%
ram_blowup_regression	0m14.236s	-1%
rollup-base-public	3m17.289s	-6%
rollup-base-private	2m57.411s	-3%
private-kernel-tail	0m1.162s	-23%
private-kernel-reset	0m8.258s	9%
private-kernel-inner	0m2.347s	-3%
parity-root	0m0.862s	-10%
noir-contracts	2m39.470s	-9%

[asterite]

Without the assert (the failing program) we get this SSA:

# Without assert
acir(inline) fn main f0 {
  b0(v0: u32, v1: u8):
    v3 = eq v1, u8 0
    v5 = lt u32 2028, v0
    enable_side_effects v3
    v6 = mul v3, v5
    enable_side_effects v6
    v8 = sub v0, u32 1
    v9 = sub v8, u32 2028
    v11 = div v9, u32 4
    v12 = add v11, u32 1
    v13 = cast v12 as i32
    v15 = lt v13, i32 0
    v16 = mul v15, v6
    constrain v16 == u1 0
    v18 = not v3
    enable_side_effects v18
    v19 = mul v18, v5
    v20 = div v9, u32 4
    enable_side_effects v19
    v21 = add v20, u32 1
    v22 = cast v21 as i32
    v23 = lt v22, i32 0
    v24 = mul v23, v19
    constrain v24 == u1 0
    enable_side_effects u1 1
    return
}

With it, we get this:

# With assert
acir(inline) fn main f0 {
  b0(v0: u32, v1: u8):
    v3 = eq v1, u8 0
    v5 = lt u32 2028, v0
    enable_side_effects v3
    v6 = mul v3, v5
    enable_side_effects v6
    v8 = sub v0, u32 1
    v9 = sub v8, u32 2028
    v11 = div v9, u32 4
    v12 = add v11, u32 1
    v13 = cast v12 as i32
    v15 = lt v13, i32 0
    v16 = mul v15, v6
    constrain v16 == u1 0
    v18 = not v3
    enable_side_effects v18
    v19 = mul v18, v5
    enable_side_effects v19
    v20 = mul v5, v19
    constrain v20 == v19
    v21 = div v9, u32 4
    v22 = add v21, u32 1
    v23 = cast v22 as i32
    v24 = lt v23, i32 0
    v25 = mul v24, v19
    constrain v25 == u1 0
    enable_side_effects u1 1
    return
}

If we rename some SSA vars so that they are the same on both sides, here's the diff we get:

[asterite]

Here's the diff if it's a println instead of an assert:

[asterite]

The failure we get in both cases is this:

error: Failed constraint
   ┌─ /Users/asterite/Sandbox/one/src/main.nr:10:21
   │
10 │             assert((((year - 1) - 2028) / 4 + 1) as i32 >= 0);
   │                     -----------------------
   │
   = Call stack:
     1. /Users/asterite/Sandbox/one/src/main.nr:10:21

[github-actions]

Changes to number of Brillig opcodes executed

Generated at commit: 820178286e8332a05e4dfe5f697847ebaf40264e, compared to commit: 0d7642cb2071fbee148978a89a0922bfffe5be6a

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
ram_blowup_regression	+98,719 ❌	+12.68%
brillig_cow_regression	+64,620 ❌	+12.45%
sha256_var_size_regression	+1,326 ❌	+8.10%
sha256_regression	+9,606 ❌	+8.09%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
ram_blowup_regression	877,369 (+98,719)	+12.68%
brillig_cow_regression	583,706 (+64,620)	+12.45%
sha256_var_size_regression	17,703 (+1,326)	+8.10%
sha256_regression	128,313 (+9,606)	+8.09%
regression_4449	214,927 (+14,070)	+7.00%
sha256_var_padding_regression	237,282 (+15,066)	+6.78%
sha256_brillig_performance_regression	24,718 (+1,524)	+6.57%
array_dynamic_blackbox_input	19,199 (+990)	+5.44%
ecdsa_secp256k1	7,158 (+369)	+5.44%
6	7,852 (+402)	+5.40%
conditional_regression_short_circuit	7,930 (+402)	+5.34%
sha256_var_witness_const_regression	7,560 (+360)	+5.00%
sha256	15,716 (+720)	+4.80%
sha2_byte	49,532 (+2,223)	+4.70%
array_dynamic_nested_blackbox_input	4,730 (+180)	+3.96%
loop_invariant_regression	1,206 (+42)	+3.61%
conditional_1	5,918 (+201)	+3.52%
keccak256	35,477 (+700)	+2.01%
regression_6674_3	1,645 (+30)	+1.86%
regression_5252	926,396 (+11,370)	+1.24%
poseidonsponge_x5_254	186,027 (+2,274)	+1.24%
poseidon_bn254_hash	164,193 (+1,599)	+0.98%
poseidon_bn254_hash_width_3	164,193 (+1,599)	+0.98%
hashmap	54,205 (+62)	+0.11%
uhashmap	145,611 (+62)	+0.04%

[github-actions]

Changes to circuit sizes

Generated at commit: 820178286e8332a05e4dfe5f697847ebaf40264e, compared to commit: 0d7642cb2071fbee148978a89a0922bfffe5be6a

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
hashmap	+4,569 ❌	+9.28%	+6,068 ❌	+5.37%
slices	+28 ❌	+3.64%	+64 ❌	+1.64%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
hashmap	53,818 (+4,569)	+9.28%	119,081 (+6,068)	+5.37%
slices	798 (+28)	+3.64%	3,962 (+64)	+1.64%
nested_array_dynamic	3,514 (+72)	+2.09%	13,033 (+100)	+0.77%
nested_array_in_slice	1,038 (+28)	+2.77%	5,589 (+39)	+0.70%
regression_struct_array_conditional	75 (+4)	+5.63%	3,208 (+5)	+0.16%
regression_3607	54 (+2)	+3.85%	2,821 (+3)	+0.11%
conditional_regression_661	38 (+2)	+5.56%	2,807 (+2)	+0.07%
sha256_var_size_regression	21,969 (+2)	+0.01%	79,870 (+4)	+0.01%
regression	319 (-12)	-3.63%	3,854 (-16)	-0.41%

[github-actions]

Execution Sample

Program	Execution Time	%
sha256_regression	0m0.617s	3%
regression_4709	0m0.388s	-2%
ram_blowup_regression	0m4.355s	-1%
rollup-base-public	0m28.148s	-5%
rollup-base-private	0m26.221s	0%
private-kernel-tail	0m0.708s	4%
private-kernel-reset	0m1.485s	-2%
private-kernel-inner	0m0.969s	-5%
parity-root	0m0.524s	1%

[asterite]

I pushed a commit just to try something and see how opcodes change.

[vezenovm]

I pushed a commit just to try something and see how opcodes change.

I think marking overflowing ops as requiring acir gen predicates is the most accurate, as these ops do in fact require the predicate. This will essentially prevent hoisting sub in the same block. We just hoist based upon the current predicate. Moving has_side_effects in the cache get would prevent more deduplications than needed.

[vezenovm]

ram_blowup_regression +98,719 ❌ +12.68%
brillig_cow_regression +64,620 ❌ +12.45%

We may be able to optimize this regression a bit in a follow-up as loop invariant code motion currently checks whether a binary operation requires a predicate inside of the check for whether an instruction can be deduplicated.

noir/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

Line 208 in 0d7642c

let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false)

Looking at the overflow check in brillig gen:

noir/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

Line 1471 in 0d7642c

fn add_overflow_check(

We can see that there are a few instructions generated during brillig gen (the Mul check is the most expensive) that we should be able to move to SSA. All instructions other than the final constrain we should then be able to hoist out of the loop.

[asterite]

I think marking overflowing ops as requiring acir gen predicates is the most accurate

Given that the name is requires_acir_gen_predicate, and that there brillig opcodes executed has increased in this PR... does this new logic is the same in acir and brillig?

[vezenovm]

Given that the name is requires_acir_gen_predicate, and that there brillig opcodes executed has increased in this PR... does this new logic is the same in acir and brillig?

It is the same in the case of loop invariant code motion. For ACIR this just helps compilation time as we unroll all loops, but this can change the final Brillig bytecode and execution trace. In both cases we do not want to be hoisting these instructions. This means we want this check:

noir/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

Line 208 in 0d7642c

let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false)

to return false.

can_be_deduplicated currently calls requires_acir_gen_predicate internally. A lot of the instructions which require a predicate have the same status in Brillig and ACIR (such as array get and checked binary ops), thus can_be_deduplicated was re-using requires_acir_gen_predicate. can_be_deduplicated in fact has more strict conditions than requires_acir_gen_predicate.

As noted above, the method to determine whether we can deduplicate instructions is muddled as it is used for both ACIR and Brillig runtimes. The intertwining of this logic has happened before and is why these methods were originally written, but we should re-assess. We may simply want better documentation surrounding these methods. We could also make a separate can_be_deduplicated method for Brillig vs. ACIR runtimes as to have a clear distinction. For example, in the Brillig runtime case, we would instead deduplicate Binary instructions based off of the deduplicate_with_predicate flag. This is more accurate for Brillig as the reason it cannot be hoisted is not due to its reliance on the acir gen predicate, but rather due to that the brillig gen overflow check code gens constrains:

noir/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

Line 1506 in 0d7642c

.codegen_constrain(condition, Some("attempt to add with overflow".to_string()));

This can happen in follow-up work though, and updating just requires_acir_gen_predicate should be fine for this PR.