chore: move unsigned overflow check from acir/brillig to ssa

compiler/noirc_evaluator/src/acir/mod.rs

@@ -1060,7 +1060,7 @@ impl<'a> Context<'a> {
 
         if let NumericType::Unsigned { bit_size } = &num_type {
             // Check for integer overflow
-            self.check_unsigned_overflow(result, *bit_size, binary, dfg, predicate)?;
+            self.check_unsigned_overflow(result, *bit_size, binary, predicate)?;
         }
 
         Ok(result)
@@ -1072,11 +1072,13 @@ impl<'a> Context<'a> {
         result: AcirVar,
         bit_size: u32,
         binary: &Binary,
-        dfg: &DataFlowGraph,
         predicate: AcirVar,
     ) -> Result<(), RuntimeError> {
-        let Some(msg) = binary.check_unsigned_overflow_msg(dfg, bit_size) else {
-            return Ok(());
+        let msg = match binary.operator {
+            BinaryOp::Add { unchecked: false } => "attempt to add with overflow",
+            BinaryOp::Sub { unchecked: false } => "attempt to subtract with overflow",
+            BinaryOp::Mul { unchecked: false } => "attempt to multiply with overflow",
+            _ => return Ok(()),
         };
 
         let with_pred = self.acir_context.mul_var(result, predicate)?;
@@ -2693,13 +2695,11 @@ mod test {
     }
 
     #[test]
-    fn multiply_with_bool_should_not_emit_range_check() {
+    fn unchecked_mul_should_not_have_range_check() {
         let src = "
             acir(inline) fn main f0 {
-            b0(v0: bool, v1: u32):
-                enable_side_effects v0
-                v2 = cast v0 as u32
-                v3 = mul v2, v1
+            b0(v0: u32, v1: u32):
+                v3 = unchecked_mul v0, v1
                 return v3
             }
         ";

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

@@ -1534,15 +1534,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
 
         self.brillig_context.binary_instruction(left, right, result_variable, brillig_binary_op);
 
-        self.add_overflow_check(
-            brillig_binary_op,
-            left,
-            right,
-            result_variable,
-            binary,
-            dfg,
-            is_signed,
-        );
+        self.add_overflow_check(left, right, result_variable, binary, dfg, is_signed);
     }
 
     fn convert_signed_modulo(
@@ -1657,7 +1649,6 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
     #[allow(clippy::too_many_arguments)]
     fn add_overflow_check(
         &mut self,
-        binary_operation: BrilligBinaryOp,
         left: SingleAddrVariable,
         right: SingleAddrVariable,
         result: SingleAddrVariable,
@@ -1671,78 +1662,73 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
             return;
         }
 
-        if let Some(msg) = binary.check_unsigned_overflow_msg(dfg, bit_size) {
-            match binary_operation {
-                BrilligBinaryOp::Add => {
-                    let condition =
-                        SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
-                    // Check that lhs <= result
-                    self.brillig_context.binary_instruction(
-                        left,
-                        result,
-                        condition,
-                        BrilligBinaryOp::LessThanEquals,
-                    );
-                    self.brillig_context.codegen_constrain(condition, Some(msg.to_string()));
-                    self.brillig_context.deallocate_single_addr(condition);
-                }
-                BrilligBinaryOp::Sub => {
-                    let condition =
+        match binary.operator {
+            BinaryOp::Add { unchecked: false } => {
+                let condition =
+                    SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
+                // Check that lhs <= result
+                self.brillig_context.binary_instruction(
+                    left,
+                    result,
+                    condition,
+                    BrilligBinaryOp::LessThanEquals,
+                );
+                let msg = "attempt to add with overflow".to_string();
+                self.brillig_context.codegen_constrain(condition, Some(msg));
+                self.brillig_context.deallocate_single_addr(condition);
+            }
+            BinaryOp::Sub { unchecked: false } => {
+                let condition =
+                    SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
+                // Check that rhs <= lhs
+                self.brillig_context.binary_instruction(
+                    right,
+                    left,
+                    condition,
+                    BrilligBinaryOp::LessThanEquals,
+                );
+                let msg = "attempt to subtract with overflow".to_string();
+                self.brillig_context.codegen_constrain(condition, Some(msg));
+                self.brillig_context.deallocate_single_addr(condition);
+            }
+            BinaryOp::Mul { unchecked: false } => {
+                let division_by_rhs_gives_lhs = |ctx: &mut BrilligContext<
+                    FieldElement,
+                    Registers,
+                >| {
+                    let condition = SingleAddrVariable::new(ctx.allocate_register(), 1);
+                    let division = SingleAddrVariable::new(ctx.allocate_register(), bit_size);
+                    // Check that result / rhs == lhs
+                    ctx.binary_instruction(result, right, division, BrilligBinaryOp::UnsignedDiv);
+                    ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals);
+                    let msg = "attempt to multiply with overflow".to_string();
+                    ctx.codegen_constrain(condition, Some(msg));
+                    ctx.deallocate_single_addr(condition);
+                    ctx.deallocate_single_addr(division);
+                };
+
+                let rhs_may_be_zero =
+                    dfg.get_numeric_constant(binary.rhs).is_none_or(|rhs| rhs.is_zero());
+                if rhs_may_be_zero {
+                    let is_right_zero =
                         SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
-                    // Check that rhs <= lhs
+                    let zero =
+                        self.brillig_context.make_constant_instruction(0_usize.into(), bit_size);
                     self.brillig_context.binary_instruction(
+                        zero,
                         right,
-                        left,
-                        condition,
-                        BrilligBinaryOp::LessThanEquals,
+                        is_right_zero,
+                        BrilligBinaryOp::Equals,
                     );
-                    self.brillig_context.codegen_constrain(condition, Some(msg.to_string()));
-                    self.brillig_context.deallocate_single_addr(condition);
-                }
-                BrilligBinaryOp::Mul => {
-                    let division_by_rhs_gives_lhs = |ctx: &mut BrilligContext<
-                        FieldElement,
-                        Registers,
-                    >| {
-                        let condition = SingleAddrVariable::new(ctx.allocate_register(), 1);
-                        let division = SingleAddrVariable::new(ctx.allocate_register(), bit_size);
-                        // Check that result / rhs == lhs
-                        ctx.binary_instruction(
-                            result,
-                            right,
-                            division,
-                            BrilligBinaryOp::UnsignedDiv,
-                        );
-                        ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals);
-                        ctx.codegen_constrain(condition, Some(msg.to_string()));
-                        ctx.deallocate_single_addr(condition);
-                        ctx.deallocate_single_addr(division);
-                    };
-
-                    let rhs_may_be_zero =
-                        dfg.get_numeric_constant(binary.rhs).is_none_or(|rhs| rhs.is_zero());
-                    if rhs_may_be_zero {
-                        let is_right_zero =
-                            SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
-                        let zero = self
-                            .brillig_context
-                            .make_constant_instruction(0_usize.into(), bit_size);
-                        self.brillig_context.binary_instruction(
-                            zero,
-                            right,
-                            is_right_zero,
-                            BrilligBinaryOp::Equals,
-                        );
-                        self.brillig_context
-                            .codegen_if_not(is_right_zero.address, division_by_rhs_gives_lhs);
-                        self.brillig_context.deallocate_single_addr(is_right_zero);
-                        self.brillig_context.deallocate_single_addr(zero);
-                    } else {
-                        division_by_rhs_gives_lhs(self.brillig_context);
-                    }
+                    self.brillig_context
+                        .codegen_if_not(is_right_zero.address, division_by_rhs_gives_lhs);
+                    self.brillig_context.deallocate_single_addr(is_right_zero);
+                    self.brillig_context.deallocate_single_addr(zero);
+                } else {
+                    division_by_rhs_gives_lhs(self.brillig_context);
                 }
-                _ => {}
             }
+            _ => {}
         }
     }
 

compiler/noirc_evaluator/src/ssa.rs

@@ -197,6 +197,7 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec<SsaPass> {
         // end up using an existing constant from the globals space.
         SsaPass::new(Ssa::brillig_array_gets, "Brillig Array Get Optimizations"),
         SsaPass::new(Ssa::dead_instruction_elimination, "Dead Instruction Elimination"),
+        SsaPass::new(Ssa::checked_to_unchecked, "Checked to unchecked"),
     ]
 }
 

compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/binary.rs

@@ -25,7 +25,7 @@ pub(super) fn simplify_binary(binary: &Binary, dfg: &mut DataFlowGraph) -> Simpl
     }
 
     let operator = if lhs_type == NumericType::NativeField {
-        // Unchecked operations between fields or bools don't make sense, so we convert those to non-unchecked
+        // Unchecked operations between fields don't make sense, so we convert those to non-unchecked
         // to reduce noise and confusion in the generated SSA.
         match operator {
             BinaryOp::Add { unchecked: true } => BinaryOp::Add { unchecked: false },
@@ -34,9 +34,11 @@ pub(super) fn simplify_binary(binary: &Binary, dfg: &mut DataFlowGraph) -> Simpl
             _ => operator,
         }
     } else if lhs_type == NumericType::bool() {
-        // Unchecked mul between bools doesn't make sense, so we convert that to non-unchecked
-        if let BinaryOp::Mul { unchecked: true } = operator {
-            BinaryOp::Mul { unchecked: false }
+        // When multiplying bools there can never be an overflow so using checked or unchecked
+        // should be the same. However, acir/brillig will check overflow for unsigned operations
+        // so here we turn checked bool multiplications to unchecked.
+        if let BinaryOp::Mul { unchecked: false } = operator {
+            BinaryOp::Mul { unchecked: true }
         } else {
             operator
         }

compiler/noirc_evaluator/src/ssa/ir/dfg/simplify/constrain.rs

@@ -245,9 +245,9 @@ mod tests {
         assert_ssa_snapshot!(ssa, @r"
         acir(inline) fn main f0 {
           b0(v0: u1, v1: u1, v2: u1):
-            v3 = mul v0, v1
+            v3 = unchecked_mul v0, v1
             v4 = not v2
-            v5 = mul v3, v4
+            v5 = unchecked_mul v3, v4
             constrain v0 == u1 1
             constrain v1 == u1 1
             constrain v2 == u1 0

compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs

@@ -2,7 +2,7 @@
 use num_traits::ToPrimitive as _;
 use serde::{Deserialize, Serialize};
 
-use super::{DataFlowGraph, InstructionResultType, NumericType, Type, ValueId};
+use super::{InstructionResultType, NumericType, Type, ValueId};
 
 /// Binary Operations allowed in the IR.
 /// Aside from the comparison operators (Eq and Lt), all operators
@@ -85,49 +85,6 @@
             _ => InstructionResultType::Operand(self.lhs),
         }
     }
-
-    /// Check if unsigned overflow is possible, and if so return some message to be used if it fails.
-    pub(crate) fn check_unsigned_overflow_msg(
-        &self,
-        dfg: &DataFlowGraph,
-        bit_size: u32,
-    ) -> Option<&'static str> {
-        // We try to optimize away operations that are guaranteed not to overflow
-        let max_lhs_bits = dfg.get_value_max_num_bits(self.lhs);
-        let max_rhs_bits = dfg.get_value_max_num_bits(self.rhs);
-
-        let msg = match self.operator {
-            BinaryOp::Add { unchecked: false } => {
-                if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size {
-                    // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow.
-                    return None;
-                }
-                "attempt to add with overflow"
-            }
-            BinaryOp::Sub { unchecked: false } => {
-                if dfg.is_constant(self.lhs) && max_lhs_bits > max_rhs_bits {
-                    // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0`
-                    // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible.
-                    return None;
-                }
-                "attempt to subtract with overflow"
-            }
-            BinaryOp::Mul { unchecked: false } => {
-                if bit_size == 1
-                    || max_lhs_bits + max_rhs_bits <= bit_size
-                    || max_lhs_bits == 1
-                    || max_rhs_bits == 1
-                {
-                    // Either performing boolean multiplication (which cannot overflow),
-                    // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow.
-                    return None;
-                }
-                "attempt to multiply with overflow"
-            }
-            _ => return None,
-        };
-        Some(msg)
-    }
 }
 
 /// Evaluate a binary operation with constant arguments.
@@ -361,7 +318,7 @@
 
             let integer_modulus = BigUint::from(2_u128).pow(bit_size);
             let truncated_as_bigint = BigUint::from(input)
                         .modpow(&BigUint::one(), &integer_modulus);
             let truncated_as_bigint = FieldElement::from_be_bytes_reduce(&truncated_as_bigint.to_bytes_be());
             prop_assert_eq!(truncated_as_field, truncated_as_bigint);
         }