From ee0754b1c6b36961c180901db59dd593c183de77 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 19 Dec 2024 13:30:56 -0300 Subject: [PATCH] fix: don't deduplicate binary math of unsigned types (#6848) Co-authored-by: Maxim Vezenov --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 24 ++++- .../src/ssa/opt/loop_invariant.rs | 92 +++++++++---------- .../regression_6834/Nargo.toml | 7 ++ .../regression_6834/Prover.toml | 2 + .../regression_6834/src/main.nr | 9 ++ 5 files changed, 83 insertions(+), 51 deletions(-) create mode 100644 test_programs/execution_success/regression_6834/Nargo.toml create mode 100644 test_programs/execution_success/regression_6834/Prover.toml create mode 100644 test_programs/execution_success/regression_6834/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index a52ba60c13a..fccd3b87d3a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -548,10 +548,25 @@ impl Instruction { /// If true the instruction will depend on `enable_side_effects` context during acir-gen. pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool { match self { - Instruction::Binary(binary) - if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) => - { - true + Instruction::Binary(binary) => { + match binary.operator { + BinaryOp::Add + | BinaryOp::Sub + | BinaryOp::Mul + | BinaryOp::Div + | BinaryOp::Mod => { + // Some binary math can overflow or underflow, but this is only the case + // for unsigned types (here we assume the type of binary.lhs is the same) + dfg.type_of_value(binary.rhs).is_unsigned() + } + BinaryOp::Eq + | BinaryOp::Lt + | BinaryOp::And + | BinaryOp::Or + | BinaryOp::Xor + | BinaryOp::Shl + | BinaryOp::Shr => false, + } } Instruction::ArrayGet { array, index } => { @@ -569,7 +584,6 @@ impl Instruction { _ => false, }, Instruction::Cast(_, _) - | Instruction::Binary(_) | Instruction::Not(_) | Instruction::Truncate { .. } | Instruction::Constrain(_, _, _) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 44c0eb380c2..c188ed1f80f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -263,17 +263,17 @@ mod test { fn simple_loop_invariant_code_motion() { let src = " brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): - jmp b1(u32 0) - b1(v2: u32): - v5 = lt v2, u32 4 + b0(v0: i32, v1: i32): + jmp b1(i32 0) + b1(v2: i32): + v5 = lt v2, i32 4 jmpif v5 then: b3, else: b2 b2(): return b3(): v6 = mul v0, v1 - constrain v6 == u32 6 - v8 = add v2, u32 1 + constrain v6 == i32 6 + v8 = add v2, i32 1 jmp b1(v8) } "; @@ -287,17 +287,17 @@ mod test { // `v6 = mul v0, v1` in b3 should now be `v3 = mul v0, v1` in b0 let expected = " brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): + b0(v0: i32, v1: i32): v3 = mul v0, v1 - jmp b1(u32 0) - b1(v2: u32): - v6 = lt v2, u32 4 + jmp b1(i32 0) + b1(v2: i32): + v6 = lt v2, i32 4 jmpif v6 then: b3, else: b2 b2(): return b3(): - constrain v3 == u32 6 - v9 = add v2, u32 1 + constrain v3 == i32 6 + v9 = add v2, i32 1 jmp b1(v9) } "; @@ -312,25 +312,25 @@ mod test { // is hoisted to the parent loop's pre-header block. let src = " brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): - jmp b1(u32 0) - b1(v2: u32): - v6 = lt v2, u32 4 + b0(v0: i32, v1: i32): + jmp b1(i32 0) + b1(v2: i32): + v6 = lt v2, i32 4 jmpif v6 then: b3, else: b2 b2(): return b3(): - jmp b4(u32 0) - b4(v3: u32): - v7 = lt v3, u32 4 + jmp b4(i32 0) + b4(v3: i32): + v7 = lt v3, i32 4 jmpif v7 then: b6, else: b5 b5(): - v9 = add v2, u32 1 + v9 = add v2, i32 1 jmp b1(v9) b6(): v10 = mul v0, v1 - constrain v10 == u32 6 - v12 = add v3, u32 1 + constrain v10 == i32 6 + v12 = add v3, i32 1 jmp b4(v12) } "; @@ -344,25 +344,25 @@ mod test { // `v10 = mul v0, v1` in b6 should now be `v4 = mul v0, v1` in b0 let expected = " brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): + b0(v0: i32, v1: i32): v4 = mul v0, v1 - jmp b1(u32 0) - b1(v2: u32): - v7 = lt v2, u32 4 + jmp b1(i32 0) + b1(v2: i32): + v7 = lt v2, i32 4 jmpif v7 then: b3, else: b2 b2(): return b3(): - jmp b4(u32 0) - b4(v3: u32): - v8 = lt v3, u32 4 + jmp b4(i32 0) + b4(v3: i32): + v8 = lt v3, i32 4 jmpif v8 then: b6, else: b5 b5(): - v10 = add v2, u32 1 + v10 = add v2, i32 1 jmp b1(v10) b6(): - constrain v4 == u32 6 - v12 = add v3, u32 1 + constrain v4 == i32 6 + v12 = add v3, i32 1 jmp b4(v12) } "; @@ -386,19 +386,19 @@ mod test { // hoist `v7 = mul v6, v0`. let src = " brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): - jmp b1(u32 0) - b1(v2: u32): - v5 = lt v2, u32 4 + b0(v0: i32, v1: i32): + jmp b1(i32 0) + b1(v2: i32): + v5 = lt v2, i32 4 jmpif v5 then: b3, else: b2 b2(): return b3(): v6 = mul v0, v1 v7 = mul v6, v0 - v8 = eq v7, u32 12 - constrain v7 == u32 12 - v9 = add v2, u32 1 + v8 = eq v7, i32 12 + constrain v7 == i32 12 + v9 = add v2, i32 1 jmp b1(v9) } "; @@ -411,19 +411,19 @@ mod test { let expected = " brillig(inline) fn main f0 { - b0(v0: u32, v1: u32): + b0(v0: i32, v1: i32): v3 = mul v0, v1 v4 = mul v3, v0 - v6 = eq v4, u32 12 - jmp b1(u32 0) - b1(v2: u32): - v9 = lt v2, u32 4 + v6 = eq v4, i32 12 + jmp b1(i32 0) + b1(v2: i32): + v9 = lt v2, i32 4 jmpif v9 then: b3, else: b2 b2(): return b3(): - constrain v4 == u32 12 - v11 = add v2, u32 1 + constrain v4 == i32 12 + v11 = add v2, i32 1 jmp b1(v11) } "; diff --git a/test_programs/execution_success/regression_6834/Nargo.toml b/test_programs/execution_success/regression_6834/Nargo.toml new file mode 100644 index 00000000000..edc3257d52d --- /dev/null +++ b/test_programs/execution_success/regression_6834/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_6834" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/regression_6834/Prover.toml b/test_programs/execution_success/regression_6834/Prover.toml new file mode 100644 index 00000000000..9ef840487ad --- /dev/null +++ b/test_programs/execution_success/regression_6834/Prover.toml @@ -0,0 +1,2 @@ +year = 1 +min_age = 1 diff --git a/test_programs/execution_success/regression_6834/src/main.nr b/test_programs/execution_success/regression_6834/src/main.nr new file mode 100644 index 00000000000..cea8d163ab5 --- /dev/null +++ b/test_programs/execution_success/regression_6834/src/main.nr @@ -0,0 +1,9 @@ +fn main(year: u32, min_age: u8) -> pub u32 { + if min_age == 0 { + (year - 2) / 4 + } else if year > 1 { + (year - 2) / 4 + } else { + 0 + } +}