fix(licm): Preserve semantic ordering of side-effectual instructions when hoisting

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

@@ -20,7 +20,7 @@
 
 pub mod errors;
 mod intrinsics;
-mod tests;
+pub(crate) mod tests;
 pub mod value;
 
 use value::Value;
@@ -877,7 +877,7 @@
     }
 }
 
 macro_rules! apply_int_binop {
     ($lhs:expr, $rhs:expr, $binary:expr, $f:expr) => {{
         use value::NumericValue::*;
         match ($lhs, $rhs) {
@@ -908,7 +908,7 @@
     }};
 }
 
 macro_rules! apply_int_binop_opt {
     ($dfg:expr, $lhs:expr, $rhs:expr, $binary:expr, $f:expr) => {{
         use value::NumericValue::*;
 
@@ -1008,7 +1008,7 @@
             }));
         }
 
         // Disable this instruction if it is side-effectful and side effects are disabled.
         if !self.side_effects_enabled() && binary.requires_acir_gen_predicate(self.dfg()) {
             let zero = NumericValue::zero(lhs.get_type());
             return Ok(Value::Numeric(zero));
@@ -1025,13 +1025,13 @@
         let dfg = self.dfg();
         let result = match binary.operator {
             BinaryOp::Add { unchecked: false } => {
                 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 } => {
                 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)

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

@@ -51,7 +51,7 @@ fn expect_value_with_args(src: &str, args: Vec<Value>) -> Value {
     results.pop().unwrap()
 }
 
-fn from_constant(constant: FieldElement, typ: NumericType) -> Value {
+pub(crate) fn from_constant(constant: FieldElement, typ: NumericType) -> Value {
     Value::from_constant(constant, typ).unwrap()
 }
 

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

@@ -411,6 +411,63 @@ impl Instruction {
         }
     }
 
+    /// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory.
+    pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool {
+        use Instruction::*;
+
+        match self {
+            // These either have side-effects or interact with memory
+            EnableSideEffectsIf { .. }
+            | Allocate
+            | Load { .. }
+            | Store { .. }
+            | IncrementRc { .. }
+            | DecrementRc { .. } => true,
+
+            Call { func, .. } => match dfg[*func] {
+                Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(),
+                // Functions known to be pure have no side effects.
+                // `PureWithPredicates` functions may still have side effects.
+                Value::Function(function) => dfg.purity_of(function) != Some(Purity::Pure),
+                _ => true, // Be conservative and assume other functions can have side effects.
+            },
+
+            // These can fail.
+            Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } => true,
+
+            // This should never be side-effectual
+            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 }
+                | BinaryOp::Div
+                | BinaryOp::Mod => true,
+                BinaryOp::Add { unchecked: true }
+                | BinaryOp::Sub { unchecked: true }
+                | BinaryOp::Mul { unchecked: true }
+                | BinaryOp::Eq
+                | BinaryOp::Lt
+                | BinaryOp::And
+                | BinaryOp::Or
+                | BinaryOp::Xor
+                | BinaryOp::Shl
+                | BinaryOp::Shr => false,
+            },
+
+            // These don't have side effects
+            Cast(_, _) | Not(_) | Truncate { .. } | IfElse { .. } => false,
+
+            // `ArrayGet`s which read from "known good" indices from an array have no side effects
+            ArrayGet { array, index, offset: _ } => !dfg.is_safe_index(*index, *array),
+
+            // ArraySet has side effects
+            ArraySet { .. } => true,
+        }
+    }
+
     /// Replaces values present in this instruction with other values according to the given mapping.
     pub(crate) fn replace_values(&mut self, mapping: &ValueMapping) {
         if !mapping.is_empty() {

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

@@ -37,7 +37,7 @@ use crate::{
             dfg::{DataFlowGraph, InsertInstructionResult},
             dom::DominatorTree,
             function::{Function, FunctionId, RuntimeType},
-            instruction::{ArrayOffset, BinaryOp, Instruction, InstructionId},
+            instruction::{ArrayOffset, Instruction, InstructionId},
             types::{NumericType, Type},
             value::{Value, ValueId, ValueMapping},
         },
@@ -543,7 +543,7 @@ impl<'brillig> Context<'brillig> {
         let predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg);
         let predicate = predicate.then_some(side_effects_enabled_var);
 
-        results_for_instruction.get(&predicate)?.get(block, dom, has_side_effects(instruction, dfg))
+        results_for_instruction.get(&predicate)?.get(block, dom, instruction.has_side_effects(dfg))
     }
 
     /// Checks if the given instruction is a call to a brillig function with all constant arguments.
@@ -833,66 +833,6 @@ fn simplify(dfg: &DataFlowGraph, lhs: ValueId, rhs: ValueId) -> Option<(ValueId,
     }
 }
 
-/// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory.
-///
-/// This is similar to `can_be_deduplicated`, but it doesn't depend on whether the caller takes
-/// constraints into account, because it might not use it to isolate the side effects across branches.
-fn has_side_effects(instruction: &Instruction, dfg: &DataFlowGraph) -> bool {
-    use Instruction::*;
-
-    match instruction {
-        // These either have side-effects or interact with memory
-        EnableSideEffectsIf { .. }
-        | Allocate
-        | Load { .. }
-        | Store { .. }
-        | IncrementRc { .. }
-        | DecrementRc { .. } => true,
-
-        Call { func, .. } => match dfg[*func] {
-            Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(),
-            // Functions known to be pure have no side effects.
-            // `PureWithPredicates` functions may still have side effects.
-            Value::Function(function) => dfg.purity_of(function) != Some(Purity::Pure),
-            _ => true, // Be conservative and assume other functions can have side effects.
-        },
-
-        // These can fail.
-        Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } => true,
-
-        // This should never be side-effectual
-        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 }
-            | BinaryOp::Div
-            | BinaryOp::Mod => true,
-            BinaryOp::Add { unchecked: true }
-            | BinaryOp::Sub { unchecked: true }
-            | BinaryOp::Mul { unchecked: true }
-            | BinaryOp::Eq
-            | BinaryOp::Lt
-            | BinaryOp::And
-            | BinaryOp::Or
-            | BinaryOp::Xor
-            | BinaryOp::Shl
-            | BinaryOp::Shr => false,
-        },
-
-        // These don't have side effects
-        Cast(_, _) | Not(_) | Truncate { .. } | IfElse { .. } => false,
-
-        // `ArrayGet`s which read from "known good" indices from an array have no side effects
-        ArrayGet { array, index, offset: _ } => !dfg.is_safe_index(*index, *array),
-
-        // ArraySet has side effects
-        ArraySet { .. } => true,
-    }
-}
-
 /// Indicates if the instruction can be safely replaced with the results of another instruction with the same inputs.
 /// If `deduplicate_with_predicate` is set, we assume we're deduplicating with the instruction
 /// and its predicate, rather than just the instruction. Setting this means instructions that