Merge 4397055 into b473d99

acvm-repo/brillig/src/opcodes.rs

@@ -338,6 +338,9 @@ pub enum BinaryIntOp {
     Add,
     Sub,
     Mul,
+    CheckedAdd,
+    CheckedSub,
+    CheckedMul,
     Div,
     /// (==) equal
     Equals,

acvm-repo/brillig_vm/src/arithmetic.rs

@@ -12,6 +12,12 @@ pub(crate) enum BrilligArithmeticError {
     MismatchedRhsBitSize { rhs_bit_size: u32, op_bit_size: u32 },
     #[error("Attempted to divide by zero")]
     DivisionByZero,
+    #[error("attempt to add with overflow")]
+    OverflowAdd,
+    #[error("attempt to subtract with overflow")]
+    OverflowSub,
+    #[error("attempt to multiply with overflow")]
+    OverflowMul,
 }
 
 /// Evaluate a binary operation on two FieldElement memory values.
@@ -143,6 +149,9 @@ fn evaluate_binary_int_op_128(
                 lhs.wrapping_shr(rhs as u32)
             }
         }
+        BinaryIntOp::CheckedAdd => lhs.wrapping_add(rhs),
+        BinaryIntOp::CheckedSub => lhs.wrapping_sub(rhs),
+        BinaryIntOp::CheckedMul => lhs.wrapping_mul(rhs),
     };
     Ok(result)
 }
@@ -160,6 +169,26 @@ fn evaluate_binary_int_op_generic(
         BinaryIntOp::Add => (lhs + rhs) % bit_modulo,
         BinaryIntOp::Sub => (bit_modulo + lhs - rhs) % bit_modulo,
         BinaryIntOp::Mul => (lhs * rhs) % bit_modulo,
+        BinaryIntOp::CheckedAdd => {
+            let result = (lhs + rhs) % bit_modulo;
+            if lhs > result {
+                return Err(BrilligArithmeticError::OverflowAdd);
+            }
+            result
+        }
+        BinaryIntOp::CheckedSub => {
+            if rhs > lhs {
+                return Err(BrilligArithmeticError::OverflowSub);
+            }
+            (bit_modulo + lhs - rhs) % bit_modulo
+        }
+        BinaryIntOp::CheckedMul => {
+            let result = (lhs * rhs) % bit_modulo;
+            if rhs != 0 && result / rhs != lhs {
+                return Err(BrilligArithmeticError::OverflowMul);
+            }
+            result
+        }
         // Perform unsigned division using the modulo operation on a and b.
         BinaryIntOp::Div => {
             if rhs == 0 {

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

@@ -1218,9 +1218,27 @@ impl<'block> BrilligBlock<'block> {
                     BrilligBinaryOp::Modulo
                 }
             }
-            BinaryOp::Add => BrilligBinaryOp::Add,
-            BinaryOp::Sub => BrilligBinaryOp::Sub,
-            BinaryOp::Mul => BrilligBinaryOp::Mul,
+            BinaryOp::Add => {
+                if is_signed {
+                    BrilligBinaryOp::Add
+                } else {
+                    BrilligBinaryOp::CheckedAdd
+                }
+            }
+            BinaryOp::Sub => {
+                if is_signed {
+                    BrilligBinaryOp::Sub
+                } else {
+                    BrilligBinaryOp::CheckedSub
+                }
+            }
+            BinaryOp::Mul => {
+                if is_signed {
+                    BrilligBinaryOp::Mul
+                } else {
+                    BrilligBinaryOp::CheckedMul
+                }
+            }
             BinaryOp::Eq => BrilligBinaryOp::Equals,
             BinaryOp::Lt => {
                 if is_signed {
@@ -1238,16 +1256,6 @@ impl<'block> BrilligBlock<'block> {
         };
 
         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,
-        );
     }
 
     /// Splits a two's complement signed integer in the sign bit and the absolute value.
@@ -1401,103 +1409,6 @@ impl<'block> BrilligBlock<'block> {
         self.brillig_context.deallocate_single_addr(bias);
     }
 
-    #[allow(clippy::too_many_arguments)]
-    fn add_overflow_check(
-        &mut self,
-        binary_operation: BrilligBinaryOp,
-        left: SingleAddrVariable,
-        right: SingleAddrVariable,
-        result: SingleAddrVariable,
-        binary: &Binary,
-        dfg: &DataFlowGraph,
-        is_signed: bool,
-    ) {
-        let bit_size = left.bit_size;
-        let max_lhs_bits = dfg.get_value_max_num_bits(binary.lhs);
-        let max_rhs_bits = dfg.get_value_max_num_bits(binary.rhs);
-
-        if bit_size == FieldElement::max_num_bits() {
-            return;
-        }
-
-        match (binary_operation, is_signed) {
-            (BrilligBinaryOp::Add, false) => {
-                if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size {
-                    // `left` and `right` have both been casted up from smaller types and so cannot overflow.
-                    return;
-                }
-
-                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("attempt to add with overflow".to_string()));
-                self.brillig_context.deallocate_single_addr(condition);
-            }
-            (BrilligBinaryOp::Sub, false) => {
-                if dfg.is_constant(binary.lhs) && max_lhs_bits > max_rhs_bits {
-                    // `left` is a fixed constant and `right` is restricted such that `left - right > 0`
-                    // Note strict inequality as `right > left` while `max_lhs_bits == max_rhs_bits` is possible.
-                    return;
-                }
-
-                let condition =
-                    SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
-                // Check that rhs <= lhs
-                self.brillig_context.binary_instruction(
-                    right,
-                    left,
-                    condition,
-                    BrilligBinaryOp::LessThanEquals,
-                );
-                self.brillig_context.codegen_constrain(
-                    condition,
-                    Some("attempt to subtract with overflow".to_string()),
-                );
-                self.brillig_context.deallocate_single_addr(condition);
-            }
-            (BrilligBinaryOp::Mul, false) => {
-                if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size {
-                    // Either performing boolean multiplication (which cannot overflow),
-                    // or `left` and `right` have both been casted up from smaller types and so cannot overflow.
-                    return;
-                }
-
-                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, |ctx| {
-                    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("attempt to multiply with overflow".to_string()),
-                    );
-                    ctx.deallocate_single_addr(condition);
-                    ctx.deallocate_single_addr(division);
-                });
-                self.brillig_context.deallocate_single_addr(is_right_zero);
-                self.brillig_context.deallocate_single_addr(zero);
-            }
-            _ => {}
-        }
-    }
-
     fn initialize_constants(&mut self, constants: &[ValueId], dfg: &DataFlowGraph) {
         for &constant_id in constants {
             self.convert_ssa_value(constant_id, dfg);

compiler/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs

@@ -57,6 +57,9 @@ impl DebugToString for BrilligBinaryOp {
             BrilligBinaryOp::Add => "+".into(),
             BrilligBinaryOp::Sub => "-".into(),
             BrilligBinaryOp::Mul => "*".into(),
+            BrilligBinaryOp::CheckedAdd => "+".into(),
+            BrilligBinaryOp::CheckedSub => "-".into(),
+            BrilligBinaryOp::CheckedMul => "*".into(),
             BrilligBinaryOp::Equals => "==".into(),
             BrilligBinaryOp::FieldDiv => "f/".into(),
             BrilligBinaryOp::UnsignedDiv => "/".into(),

compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs

@@ -438,6 +438,9 @@ pub(crate) enum BrilligBinaryOp {
     Add,
     Sub,
     Mul,
+    CheckedAdd,
+    CheckedSub,
+    CheckedMul,
     FieldDiv,
     UnsignedDiv,
     Equals,
@@ -458,6 +461,9 @@ impl From<BrilligBinaryOp> for BinaryFieldOp {
             BrilligBinaryOp::Add => BinaryFieldOp::Add,
             BrilligBinaryOp::Sub => BinaryFieldOp::Sub,
             BrilligBinaryOp::Mul => BinaryFieldOp::Mul,
+            BrilligBinaryOp::CheckedAdd => BinaryFieldOp::Add,
+            BrilligBinaryOp::CheckedSub => BinaryFieldOp::Sub,
+            BrilligBinaryOp::CheckedMul => BinaryFieldOp::Mul,
             BrilligBinaryOp::FieldDiv => BinaryFieldOp::Div,
             BrilligBinaryOp::UnsignedDiv => BinaryFieldOp::IntegerDiv,
             BrilligBinaryOp::Equals => BinaryFieldOp::Equals,
@@ -474,6 +480,9 @@ impl From<BrilligBinaryOp> for BinaryIntOp {
             BrilligBinaryOp::Add => BinaryIntOp::Add,
             BrilligBinaryOp::Sub => BinaryIntOp::Sub,
             BrilligBinaryOp::Mul => BinaryIntOp::Mul,
+            BrilligBinaryOp::CheckedAdd => BinaryIntOp::CheckedAdd,
+            BrilligBinaryOp::CheckedSub => BinaryIntOp::CheckedSub,
+            BrilligBinaryOp::CheckedMul => BinaryIntOp::CheckedMul,
             BrilligBinaryOp::UnsignedDiv => BinaryIntOp::Div,
             BrilligBinaryOp::Equals => BinaryIntOp::Equals,
             BrilligBinaryOp::LessThan => BinaryIntOp::LessThan,

compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_insert.rs

@@ -135,21 +135,34 @@ pub(super) fn compile_prepare_vector_insert_procedure<F: AcirField + DebugToStri
     );
 
     // Compute the number of elements to the right of the insertion index
-    let element_count_to_the_right = brillig_context.allocate_register();
+    let element_count_to_the_right: MemoryAddress = brillig_context.allocate_register();
     brillig_context.memory_op_instruction(
         source_size.address,
         index.address,
         element_count_to_the_right,
         BrilligBinaryOp::Sub,
     );
 
-    // Copy the elements to the right of the index
-    brillig_context.codegen_mem_copy_from_the_end(
-        source_pointer_at_index,
-        target_pointer_after_index,
-        SingleAddrVariable::new_usize(element_count_to_the_right),
+    let num_elements_variable: SingleAddrVariable =
+        SingleAddrVariable::new_usize(element_count_to_the_right);
+    // brillig_context.codegen_branch(condition, f);
+    let is_num_items_zero = brillig_context.allocate_register();
+    brillig_context.codegen_usize_op(
+        num_elements_variable.address,
+        is_num_items_zero,
+        BrilligBinaryOp::Equals,
+        0,
     );
 
+    brillig_context.codegen_if_not(is_num_items_zero, |ctx| {
+        // Copy the elements to the right of the index
+        ctx.codegen_mem_copy_from_the_end(
+            source_pointer_at_index,
+            target_pointer_after_index,
+            num_elements_variable,
+        );
+    });
+
     brillig_context.deallocate_single_addr(source_rc);
     brillig_context.deallocate_single_addr(source_size);
     brillig_context.deallocate_single_addr(source_capacity);

test_programs/noir_test_success/brillig_overflow_checks/src/main.nr

@@ -1,5 +1,3 @@
-use std::field::bn254::{TWO_POW_128, assert_gt};
-
 #[test(should_fail_with = "attempt to add with overflow")]
 unconstrained fn test_overflow_add() {
     let a: u8 = 255;

tooling/profiler/src/opcode_formatter.rs

@@ -106,6 +106,9 @@ fn format_binary_int(op: &BinaryIntOp) -> String {
         BinaryIntOp::Xor => "xor".to_string(),
         BinaryIntOp::Shl => "shl".to_string(),
         BinaryIntOp::Shr => "shr".to_string(),
+        BinaryIntOp::CheckedAdd => "c_add".to_string(),
+        BinaryIntOp::CheckedSub => "c_sub".to_string(),
+        BinaryIntOp::CheckedMul => "c_mul".to_string(),
     }
 }