fix(ssa): Mislabeled instructions with side effects in EnableSideEffectsIf removal pass

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

@@ -1162,7 +1162,7 @@ mod test {
             ";
         let expected = "
             acir(inline) fn main f0 {
-              b0(v0: [Field; 4], v1: u32, v2: u1, v3: u1):
+              b0(v0: [Field; 4], v1: u32, v2: bool, v3: bool):
                 enable_side_effects v2
                 v5 = array_get v0, index u32 0 -> Field
                 v6 = array_get v0, index v1 -> Field

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

@@ -113,7 +113,7 @@
         // Set of functions we call which the purity result depends on.
         // `is_pure` is intended to be called on each function, building
         // up a call graph of sorts to check afterwards to propagate impurity
         // from called functions to their callers. Resultingly, an initial "Pure"
         // result here could be overridden by one of these dependencies being impure.
         let mut dependencies = BTreeSet::new();
 
@@ -408,7 +408,7 @@
         acir(inline) pure fn pure_basic f6 {
           b0():
             v2 = make_array [Field 0, Field 1] : [Field; 2]
-            v4 = array_get v2, index u32 1 -> Field
+            v4 = array_get v0, index u32 1 -> Field
             v5 = allocate -> &mut Field
             store Field 0 at v5
             return

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

@@ -14,11 +14,9 @@ use acvm::{FieldElement, acir::AcirField};
 
 use crate::ssa::{
     ir::{
-        dfg::DataFlowGraph,
         function::{Function, RuntimeType},
-        instruction::{BinaryOp, Hint, Instruction, Intrinsic},
+        instruction::Instruction,
         types::NumericType,
-        value::Value,
     },
     ssa_gen::Ssa,
 };
@@ -91,7 +89,7 @@ impl Function {
 
             // If we hit an instruction which is affected by the side effects var then we must insert the
             // `Instruction::EnableSideEffectsIf` before we insert this new instruction.
-            if responds_to_side_effects_var(context.dfg, instruction) {
+            if instruction.requires_acir_gen_predicate(context.dfg) {
                 if let Some(enable_side_effects_instruction_id) =
                     last_side_effects_enabled_instruction.take()
                 {
@@ -102,79 +100,6 @@ impl Function {
     }
 }
 
-fn responds_to_side_effects_var(dfg: &DataFlowGraph, instruction: &Instruction) -> bool {
-    use Instruction::*;
-    match instruction {
-        Binary(binary) => match binary.operator {
-            BinaryOp::Add { .. } | BinaryOp::Sub { .. } | BinaryOp::Mul { .. } => {
-                dfg.type_of_value(binary.lhs).is_unsigned()
-            }
-            BinaryOp::Div | BinaryOp::Mod => {
-                if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) {
-                    rhs == FieldElement::zero()
-                } else {
-                    true
-                }
-            }
-            _ => false,
-        },
-
-        Cast(_, _)
-        | Not(_)
-        | Truncate { .. }
-        | Constrain(..)
-        | ConstrainNotEqual(..)
-        | RangeCheck { .. }
-        | IfElse { .. }
-        | IncrementRc { .. }
-        | DecrementRc { .. }
-        | Noop
-        | MakeArray { .. } => false,
-
-        EnableSideEffectsIf { .. }
-        | ArrayGet { .. }
-        | ArraySet { .. }
-        | Allocate
-        | Store { .. }
-        | Load { .. } => true,
-
-        // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
-        Call { func, .. } => match dfg[*func] {
-            Value::Intrinsic(intrinsic) => match intrinsic {
-                Intrinsic::SlicePushBack
-                | Intrinsic::SlicePushFront
-                | Intrinsic::SlicePopBack
-                | Intrinsic::SlicePopFront
-                | Intrinsic::SliceInsert
-                | Intrinsic::SliceRemove => true,
-
-                Intrinsic::ArrayLen
-                | Intrinsic::ArrayAsStrUnchecked
-                | Intrinsic::AssertConstant
-                | Intrinsic::StaticAssert
-                | Intrinsic::ApplyRangeConstraint
-                | Intrinsic::StrAsBytes
-                | Intrinsic::ToBits(_)
-                | Intrinsic::ToRadix(_)
-                | Intrinsic::BlackBox(_)
-                | Intrinsic::Hint(Hint::BlackBox)
-                | Intrinsic::AsSlice
-                | Intrinsic::AsWitness
-                | Intrinsic::IsUnconstrained
-                | Intrinsic::DerivePedersenGenerators
-                | Intrinsic::ArrayRefCount
-                | Intrinsic::SliceRefCount
-                | Intrinsic::FieldLessThan => false,
-            },
-
-            // We must assume that functions contain a side effect as we cannot inspect more deeply.
-            Value::Function(_) => true,
-
-            _ => false,
-        },
-    }
-}
-
 #[cfg(test)]
 mod test {
     use crate::{assert_ssa_snapshot, ssa::ssa_gen::Ssa};
@@ -216,34 +141,81 @@ mod test {
     fn keeps_enable_side_effects_for_instructions_that_have_side_effects() {
         let src = "
         acir(inline) fn main f0 {
-          b0(v0: Field, v1: Field):
-            enable_side_effects v0
-            v3 = allocate -> &mut Field
-            enable_side_effects v0
-            v4 = allocate -> &mut Field
+          b0(v0: [u16; 3], v1: u32, v2: u32):
             enable_side_effects v1
-            v5 = allocate -> &mut Field
+            v3 = array_get v0, index v1 -> u16
+            enable_side_effects v1
+            v4 = array_get v0, index v1 -> u16
+            enable_side_effects v2
+            v5 = array_get v0, index v1 -> u16
             enable_side_effects u1 1
-            v6 = allocate -> &mut Field
+            v7 = array_get v0, index v1 -> u16
             return
-        }
-        ";
+        }";
         let ssa = Ssa::from_str(src).unwrap();
 
         let ssa = ssa.remove_enable_side_effects();
         assert_ssa_snapshot!(ssa, @r"
         acir(inline) fn main f0 {
-          b0(v0: Field, v1: Field):
-            enable_side_effects v0
-            v2 = allocate -> &mut Field
-            v3 = allocate -> &mut Field
+          b0(v0: [u16; 3], v1: u32, v2: u32):
             enable_side_effects v1
-            v4 = allocate -> &mut Field
+            v3 = array_get v0, index v1 -> u16
+            v4 = array_get v0, index v1 -> u16
+            enable_side_effects v2
+            v5 = array_get v0, index v1 -> u16
             enable_side_effects u1 1
-            v6 = allocate -> &mut Field
+            v7 = array_get v0, index v1 -> u16
             return
+        }");
+    }
+
+    #[test]
+    fn keep_enable_side_effects_for_safe_modulo() {
+        let src = 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
+            enable_side_effects v8
+            v9 = not v8
+            enable_side_effects v9
+            v11 = mod v7, u32 3
+            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
+        }
+        "#;
+
+        let ssa = Ssa::from_str(src).unwrap();
+        let ssa = ssa.remove_enable_side_effects();
+
+        // We expect the SSA to be unchanged
+        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
+            enable_side_effects v9
+            v11 = mod v7, u32 3
+            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_programs/execution_success/regression_8236/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "regression_8236"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_success/regression_8236/Prover.toml

@@ -0,0 +1,6 @@
+a = [
+    "0x000000000000000000000000000000000000000000000000000000000000afdb",
+    "0x000000000000000000000000000000000000000000000000000000000000472d",
+    "0x00000000000000000000000000000000000000000000000000000000000046ce",
+]
+b = [false]

test_programs/execution_success/regression_8236/src/main.nr

@@ -0,0 +1,16 @@
+global G_A: [u16; 3] = [33700, 47314, 35095];
+global G_B: [u16; 3] = [59890, 17417, 14409];
+fn main(a: [u16; 3], b: [bool; 1]) -> pub bool {
+    // Safety: testing context
+    let res = unsafe { func_1(G_B, true) }[(((a[0] as u32) % (G_B[2] as u32)) % 1)];
+    if res {
+        // Safety: testing context
+        let c = unsafe { func_1(a, b[0]) };
+        b[0]
+    } else {
+        ((a[((a[0] as u32) % 3)] as u32) > ((24993 % G_A[1]) as u32))
+    }
+}
+unconstrained fn func_1(a: [u16; 3], b: bool) -> [bool; 1] {
+    [false]
+}

test_programs/execution_success/regression_8236/stdout.txt

@@ -0,0 +1 @@
+[regression_8236] Circuit output: Field(0)