feat: safe division has no side effects

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

@@ -450,7 +450,7 @@ 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) => binary.requires_acir_gen_predicate(dfg),
+            Instruction::Binary(binary) => binary.has_side_effects(dfg),
 
             Instruction::ArrayGet { array, index } => {
                 // `ArrayGet`s which read from "known good" indices from an array should not need a predicate.
@@ -528,53 +528,7 @@ impl Instruction {
             MakeArray { .. } | Noop => false,
 
             // Some binary math can overflow or underflow.
-            Binary(binary) => match binary.operator {
-                BinaryOp::Add { unchecked: false }
-                | BinaryOp::Sub { unchecked: false }
-                | BinaryOp::Mul { unchecked: false } => {
-                    let typ = dfg.type_of_value(binary.lhs);
-                    !matches!(typ, Type::Numeric(NumericType::NativeField))
-                }
-                BinaryOp::Div | BinaryOp::Mod => {
-                    // If we don't know rhs at compile time, it might be zero or -1
-                    let Some(rhs) = dfg.get_numeric_constant(binary.rhs) else {
-                        return true;
-                    };
-
-                    // Div or mod by zero is a side effect (failure)
-                    if rhs.is_zero() {
-                        return true;
-                    }
-
-                    // For signed types, division or modulo by -1 can overflow.
-                    let typ = dfg.type_of_value(binary.rhs).unwrap_numeric();
-                    let NumericType::Signed { bit_size } = typ else {
-                        return false;
-                    };
-
-                    let minus_one = IntegerConstant::Signed { value: -1, bit_size };
-                    if IntegerConstant::from_numeric_constant(rhs, typ) == Some(minus_one) {
-                        return true;
-                    }
-
-                    false
-                }
-                BinaryOp::Shl | BinaryOp::Shr => {
-                    // Bit-shifts which are known to be by a number of bits less than the bit size of the type have no side effects.
-                    dfg.get_numeric_constant(binary.rhs).is_none_or(|c| {
-                        let typ = dfg.type_of_value(binary.lhs);
-                        c >= typ.bit_size().into()
-                    })
-                }
-                BinaryOp::Add { unchecked: true }
-                | BinaryOp::Sub { unchecked: true }
-                | BinaryOp::Mul { unchecked: true }
-                | BinaryOp::Eq
-                | BinaryOp::Lt
-                | BinaryOp::And
-                | BinaryOp::Or
-                | BinaryOp::Xor => false,
-            },
+            Binary(binary) => binary.has_side_effects(dfg),
 
             // These don't have side effects
             Cast(_, _) | Not(_) | Truncate { .. } | IfElse { .. } => false,
@@ -864,20 +818,37 @@ impl ArrayOffset {
 }
 
 impl Binary {
-    pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool {
+    pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool {
         match self.operator {
             BinaryOp::Add { unchecked: false }
             | BinaryOp::Sub { unchecked: false }
             | BinaryOp::Mul { unchecked: false } => {
-                match dfg.type_of_value(self.rhs).unwrap_numeric() {
-                    NumericType::NativeField => false,
-                    // Some binary math can overflow or underflow for non-field types.
-                    NumericType::Unsigned { .. } => true,
-                    // However, we assume that signed types should have already been expanded using unsigned operations.
-                    NumericType::Signed { .. } => {
-                        unreachable!("signed instructions should have been already expanded")
-                    }
+                let typ = dfg.type_of_value(self.lhs);
+                !matches!(typ, Type::Numeric(NumericType::NativeField))
+            }
+            BinaryOp::Div | BinaryOp::Mod => {
+                // If we don't know rhs at compile time, it might be zero or -1
+                let Some(rhs) = dfg.get_numeric_constant(self.rhs) else {
+                    return true;
+                };
+
+                // Div or mod by zero is a side effect (failure)
+                if rhs.is_zero() {
+                    return true;
                 }
+
+                // For signed types, division or modulo by -1 can overflow.
+                let typ = dfg.type_of_value(self.rhs).unwrap_numeric();
+                let NumericType::Signed { bit_size } = typ else {
+                    return false;
+                };
+
+                let minus_one = IntegerConstant::Signed { value: -1, bit_size };
+                if IntegerConstant::from_numeric_constant(rhs, typ) == Some(minus_one) {
+                    return true;
+                }
+
+                false
             }
             BinaryOp::Shl | BinaryOp::Shr => {
                 // Bit-shifts which are known to be by a number of bits less than the bit size of the type have no side effects.
@@ -886,7 +857,6 @@ impl Binary {
                     c >= typ.bit_size().into()
                 })
             }
-            BinaryOp::Div | BinaryOp::Mod => true,
             BinaryOp::Add { unchecked: true }
             | BinaryOp::Sub { unchecked: true }
             | BinaryOp::Mul { unchecked: true }

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

@@ -2190,8 +2190,7 @@ mod test {
     }
 
     #[test]
-    fn does_not_duplicate_unsigned_division_by_non_zero_constant() {
-        // Regression test for https://github.com/noir-lang/noir/issues/7836
+    fn deduplicates_unsigned_division_by_non_zero_constant() {
         let src = "
         acir(inline) fn main f0 {
           b0(v0: u32, v1: u32, v2: u1):
@@ -2203,6 +2202,33 @@ mod test {
             return
         }
         ";
+        let ssa = Ssa::from_str(src).unwrap();
+        let ssa = ssa.fold_constants(MIN_ITER);
+        assert_ssa_snapshot!(ssa, @r"
+        acir(inline) fn main f0 {
+          b0(v0: u32, v1: u32, v2: u1):
+            enable_side_effects v2
+            v4 = div v1, u32 2
+            v5 = not v2
+            enable_side_effects v5
+            return
+        }
+        ");
+    }
+
+    #[test]
+    fn does_not_deduplicate_signed_division_by_minus_one_constant() {
+        let src = "
+        acir(inline) fn main f0 {
+          b0(v0: i32, v1: i32, v2: u1):
+            enable_side_effects v2
+            v4 = div v1, i32 -1
+            v5 = not v2
+            enable_side_effects v5
+            v6 = div v1, i32 -1
+            return
+        }
+        ";
         assert_ssa_does_not_change(src, |ssa| ssa.fold_constants(MIN_ITER));
     }
 

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

@@ -56,8 +56,8 @@ impl LoopInvariantContext<'_> {
             // An unchecked operation cannot overflow, so it can be safely evaluated.
             // Some checked operations can be safely evaluated, depending on the loop bounds, but in that case,
             // they would have been already converted to unchecked operation in `simplify_induction_variable_in_binary()`.
-            // These are all handled by `requires_acir_gen_predicate`, and are redundant with `can_be_hoisted`.
-            _ => !binary.requires_acir_gen_predicate(&self.inserter.function.dfg),
+            // These are all handled by `has_side_effects`, and are redundant with `can_be_hoisted`.
+            _ => !binary.has_side_effects(&self.inserter.function.dfg),
         }
     }
 

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

@@ -238,7 +238,33 @@ mod test {
     }
 
     #[test]
-    fn keep_enable_side_effects_for_safe_modulo() {
+    fn keeps_enable_side_effects_for_unsafe_modulo() {
+        // This is a simplification of `test_programs/execution_success/regression_8236`
+        let src = r#"
+        acir(inline) predicate_pure fn main f0 {
+          b0(v0: [u16; 3], v1: [u1; 1], v2: u32):
+            v4 = call f1(v0, u1 1) -> [u1; 1]
+            v6 = array_get v0, index u32 0 -> u16
+            v7 = cast v6 as u32
+            v8 = array_get v4, index u32 0 -> u1
+            v9 = not v8
+            enable_side_effects v9
+            v11 = mod v7, v2
+            v12 = array_get v0, index v11 -> u16
+            enable_side_effects u1 1
+            return v12
+        }
+        brillig(inline) predicate_pure fn func_1 f1 {
+          b0(v0: [u16; 3], v1: u1):
+            v3 = make_array [u1 0] : [u1; 1]
+            return v3
+        }
+        "#;
+        assert_ssa_does_not_change(src, Ssa::remove_enable_side_effects);
+    }
+
+    #[test]
+    fn does_not_keep_enable_side_effects_for_safe_modulo() {
         // This is a simplification of `test_programs/execution_success/regression_8236`
         let src = r#"
         acir(inline) predicate_pure fn main f0 {
@@ -260,11 +286,47 @@ mod test {
             return v3
         }
         "#;
+
+        let ssa = Ssa::from_str(src).unwrap();
+        let ssa = ssa.remove_enable_side_effects();
+        assert_ssa_snapshot!(ssa, @r"
+        acir(inline) predicate_pure fn main f0 {
+          b0(v0: [u16; 3], v1: [u1; 1]):
+            v4 = call f1(v0, u1 1) -> [u1; 1]
+            v6 = array_get v0, index u32 0 -> u16
+            v7 = cast v6 as u32
+            v8 = array_get v4, index u32 0 -> u1
+            v9 = not v8
+            v11 = mod v7, u32 3
+            enable_side_effects v9
+            v12 = array_get v0, index v11 -> u16
+            enable_side_effects u1 1
+            return v12
+        }
+        brillig(inline) predicate_pure fn func_1 f1 {
+          b0(v0: [u16; 3], v1: u1):
+            v3 = make_array [u1 0] : [u1; 1]
+            return v3
+        }
+        ");
+    }
+
+    #[test]
+    fn keeps_side_effects_for_unsafe_div() {
+        let src = r#"
+        acir(inline) predicate_pure fn main f0 {
+          b0(v0: [u32; 3], v1: u1, v2: u32):
+            v3 = array_get v0, index u32 0 -> u32
+            enable_side_effects v1
+            v5 = div v3, v2
+            return
+        }
+        "#;
         assert_ssa_does_not_change(src, Ssa::remove_enable_side_effects);
     }
 
     #[test]
-    fn keep_side_effects_for_safe_div() {
+    fn does_not_keep_side_effects_for_safe_div() {
         let src = r#"
         acir(inline) predicate_pure fn main f0 {
           b0(v0: [u32; 3], v1: u1):
@@ -274,7 +336,16 @@ mod test {
             return
         }
         "#;
-        assert_ssa_does_not_change(src, Ssa::remove_enable_side_effects);
+        let ssa = Ssa::from_str(src).unwrap();
+        let ssa = ssa.remove_enable_side_effects();
+        assert_ssa_snapshot!(ssa, @r"
+        acir(inline) predicate_pure fn main f0 {
+          b0(v0: [u32; 3], v1: u1):
+            v3 = array_get v0, index u32 0 -> u32
+            v5 = div v3, u32 3
+            return
+        }
+        ");
     }
 
     #[test]

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

@@ -273,8 +273,7 @@
                     };
 
                     // Check if this operation is one that should only fail if the predicate is enabled.
-                    let requires_acir_gen_predicate =
-                        binary.requires_acir_gen_predicate(context.dfg);
+                    let requires_acir_gen_predicate = binary.has_side_effects(context.dfg);
 
                     let fails_under_predicate =
                         requires_acir_gen_predicate && !is_predicate_constant_one;
@@ -1363,9 +1362,7 @@
     fn simplifies_instructions_following_conditional_failure() {
         // In the following SSA we have:
         // 1. v1 is a divide-by-zero which turns into an conditional-fail constraint under v0
-        // 2. v2 is replaced by its default value (because division is considered side effecting)
-        // 3. v3 would be turned into a `truncate u64 0 to 32 bits` due to step 2, which is not expected to reach ACIR gen.
-        // We expect 3 to disappear as it can be simplified out.
+        // 2. v2 is kept, as it's not side-effectuful
         let src = "
         acir(inline) predicate_pure fn main f0 {
           b0(v0: u1):
@@ -1386,6 +1383,8 @@
           b0(v0: u1):
             enable_side_effects v0
             constrain u1 0 == v0, "attempt to divide by zero"
+            v3 = div u64 1, u64 1
+            v4 = truncate v3 to 32 bits, max_bit_size: 254
             enable_side_effects u1 1
             return
         }