feat: push overflow checks inside of signed binary ops

.github/benchmark_projects.yml

@@ -73,7 +73,7 @@ projects:
     path: noir-projects/noir-protocol-circuits/crates/rollup-block-root
     num_runs: 1
     timeout: 60
-    compilation-timeout: 200
+    compilation-timeout: 250
     execution-timeout: 40
     compilation-memory-limit: 10000
     execution-memory-limit: 1900

compiler/noirc_evaluator/src/acir/mod.rs

@@ -1044,6 +1044,18 @@ impl<'a> Context<'a> {
         let lhs = self.convert_numeric_value(binary.lhs, dfg)?;
         let rhs = self.convert_numeric_value(binary.rhs, dfg)?;
         let binary_type = self.type_of_binary_operation(binary, dfg);
+
+        if binary_type.is_signed()
+            && matches!(
+                binary.operator,
+                BinaryOp::Add { unchecked: false }
+                    | BinaryOp::Sub { unchecked: false }
+                    | BinaryOp::Mul { unchecked: false }
+            )
+        {
+            panic!("Checked signed operations should all be removed before ACIRgen")
+        }
+
         let binary_type = AcirType::from(binary_type);
         let bit_count = binary_type.bit_size::<FieldElement>();
         let num_type = binary_type.to_numeric_type();

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

@@ -1761,6 +1761,16 @@
         let bit_size = left.bit_size;
 
         if bit_size == FieldElement::max_num_bits() || is_signed {
+            if is_signed
+                && matches!(
+                    binary.operator,
+                    BinaryOp::Add { unchecked: false }
+                        | BinaryOp::Sub { unchecked: false }
+                        | BinaryOp::Mul { unchecked: false }
+                )
+            {
+                panic!("Checked signed operations should all be removed before brilliggen")
+            }
             return;
         }
 

compiler/noirc_evaluator/src/ssa/interpreter/errors.rs

@@ -55,7 +55,7 @@ pub enum InterpreterError {
     )]
     IncRcRevive { value_id: ValueId, value: String },
     #[error("An overflow occurred while evaluating {instruction}")]
-    Overflow { instruction: String },
+    Overflow { operator: BinaryOp, instruction: String },
     #[error(
         "if-else instruction with then condition `{then_condition_id}` and else condition `{else_condition_id}` has both branches as true. This should be impossible except for malformed SSA code"
     )]

compiler/noirc_evaluator/src/ssa/interpreter/mod.rs

@@ -1096,7 +1096,7 @@ macro_rules! apply_int_binop_opt {
         let operator = binary.operator;
 
         let overflow = || {
-            if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) {
+            if matches!(operator, BinaryOp::Div | BinaryOp::Mod) {
                 let lhs_id = binary.lhs;
                 let rhs_id = binary.rhs;
                 let lhs = lhs.to_string();
@@ -1105,7 +1105,7 @@ macro_rules! apply_int_binop_opt {
             } else {
                 let instruction =
                     format!("`{}` ({operator} {lhs}, {rhs})", display_binary(binary, $dfg));
-                InterpreterError::Overflow { instruction }
+                InterpreterError::Overflow { operator, instruction }
             }
         };
 
@@ -1186,7 +1186,7 @@ impl<W: Write> Interpreter<'_, W> {
             }));
         }
 
-        // Disable this instruction if it is side-effectful and side effects are disabled.
+        // Disable this instruction if it is side-effectual and side effects are disabled.
         if !side_effects_enabled {
             let zero = NumericValue::zero(lhs.get_type());
             return Ok(Value::Numeric(zero));
@@ -1203,32 +1203,20 @@ impl<W: Write> Interpreter<'_, W> {
         let dfg = self.dfg();
         let result = match binary.operator {
             BinaryOp::Add { unchecked: false } => {
-                if lhs.get_type().is_unsigned() {
-                    apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedAdd::checked_add)
-                } else {
-                    apply_int_binop!(lhs, rhs, binary, num_traits::WrappingAdd::wrapping_add)
-                }
+                apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedAdd::checked_add)
             }
             BinaryOp::Add { unchecked: true } => {
                 apply_int_binop!(lhs, rhs, binary, num_traits::WrappingAdd::wrapping_add)
             }
             BinaryOp::Sub { unchecked: false } => {
-                if lhs.get_type().is_unsigned() {
-                    apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedSub::checked_sub)
-                } else {
-                    apply_int_binop!(lhs, rhs, binary, num_traits::WrappingSub::wrapping_sub)
-                }
+                apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedSub::checked_sub)
             }
             BinaryOp::Sub { unchecked: true } => {
                 apply_int_binop!(lhs, rhs, binary, num_traits::WrappingSub::wrapping_sub)
             }
             BinaryOp::Mul { unchecked: false } => {
                 // Only unsigned multiplication has side effects
-                if lhs.get_type().is_unsigned() {
-                    apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedMul::checked_mul)
-                } else {
-                    apply_int_binop!(lhs, rhs, binary, num_traits::WrappingMul::wrapping_mul)
-                }
+                apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedMul::checked_mul)
             }
             BinaryOp::Mul { unchecked: true } => {
                 apply_int_binop!(lhs, rhs, binary, num_traits::WrappingMul::wrapping_mul)
@@ -1364,7 +1352,8 @@ impl<W: Write> Interpreter<'_, W> {
     fn interpret_u1_binary_op(&mut self, lhs: bool, rhs: bool, binary: &Binary) -> IResult<Value> {
         let overflow = || {
             let instruction = format!("`{}` ({lhs} << {rhs})", display_binary(binary, self.dfg()));
-            InterpreterError::Overflow { instruction }
+            let operator = binary.operator;
+            InterpreterError::Overflow { operator, instruction }
         };
 
         let lhs_id = binary.lhs;

compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs

@@ -66,7 +66,7 @@ fn add_overflow_unsigned() {
 
 #[test]
 fn add_overflow_signed() {
-    let value = expect_value(
+    let error = expect_error(
         "
         acir(inline) fn main f0 {
           b0():
@@ -76,7 +76,7 @@ fn add_overflow_signed() {
         }
     ",
     );
-    assert_eq!(value, Value::Numeric(NumericValue::I8(-127)));
+    assert!(matches!(error, InterpreterError::Overflow { .. }));
 }
 
 #[test]
@@ -152,7 +152,7 @@ fn sub_underflow_unsigned() {
 
 #[test]
 fn sub_underflow_signed() {
-    let value = expect_value(
+    let error = expect_error(
         "
         acir(inline) fn main f0 {
           b0():
@@ -162,10 +162,7 @@ fn sub_underflow_signed() {
         }
     ",
     );
-    // Expected wrapping sub:
-    // i8 can only be -128 to 127
-    // -120 - 10 = -130 = 126
-    assert!(matches!(value, Value::Numeric(NumericValue::I8(126))));
+    assert!(matches!(error, InterpreterError::Overflow { .. }));
 }
 
 #[test]
@@ -243,22 +240,16 @@ fn mul_overflow_unsigned() {
 
 #[test]
 fn mul_overflow_signed() {
-    // We return v0 as we simply want the output from the mul operation in this test.
-    // However, the valid SSA signed overflow patterns requires that the appropriate
-    // casts and truncates follow a signed mul.
-    let value = expect_value(
+    let error = expect_error(
         "
         acir(inline) fn main f0 {
           b0():
             v0 = mul i8 127, i8 2
-            v1 = cast v0 as u16
-            v2 = truncate v1 to 8 bits, max_bit_size: 16
-            v3 = cast v2 as i8
             return v0
         }
     ",
     );
-    assert_eq!(value, Value::Numeric(NumericValue::I8(-2)));
+    assert!(matches!(error, InterpreterError::Overflow { .. }));
 }
 
 #[test]

compiler/noirc_evaluator/src/ssa/ir/types.rs

@@ -155,6 +155,11 @@ impl Type {
         matches!(self, Type::Numeric(NumericType::Unsigned { .. }))
     }
 
+    /// Returns whether the `Type` represents an signed numeric type.
+    pub fn is_signed(&self) -> bool {
+        matches!(self, Type::Numeric(NumericType::Signed { .. }))
+    }
+
     /// Create a new signed integer type with the given amount of bits.
     pub fn signed(bit_size: u32) -> Type {
         Type::Numeric(NumericType::Signed { bit_size })

compiler/noirc_evaluator/src/ssa/mod.rs

@@ -107,6 +107,7 @@ pub struct ArtifactsAndWarnings(pub Artifacts, pub Vec<SsaReport>);
 /// something we take can advantage of in the [secondary_passes].
 pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec<SsaPass> {
     vec![
+        SsaPass::new(Ssa::expand_signed_checks, "expand signed checks"),
         SsaPass::new(Ssa::remove_unreachable_functions, "Removing Unreachable Functions"),
         SsaPass::new(Ssa::defunctionalize, "Defunctionalization"),
         SsaPass::new(Ssa::inline_simple_functions, "Inlining simple functions")
@@ -219,6 +220,8 @@ pub fn secondary_passes(brillig: &Brillig) -> Vec<SsaPass> {
 /// In the future, we can potentially execute the actual initial version using the SSA interpreter.
 pub fn minimal_passes() -> Vec<SsaPass<'static>> {
     vec![
+        // Signed integer operations need to be expanded in order to have the appropriate overflow checks applied.
+        SsaPass::new(Ssa::expand_signed_checks, "expand signed checks"),
         // We need to get rid of function pointer parameters, otherwise they cause panic in Brillig generation.
         SsaPass::new(Ssa::defunctionalize, "Defunctionalization"),
         // Even the initial SSA generation can result in optimizations that leave a function

compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs

@@ -1454,7 +1454,7 @@ mod test {
 
             brillig(inline) fn one f1 {
               b0(v0: i32, v1: i32):
-                v2 = add v0, v1
+                v2 = unchecked_add v0, v1
                 v3 = truncate v2 to 32 bits, max_bit_size: 33
                 return v3
             }