chore: rename instruction checks for side effects (noir-lang#4945)

# Description

## Problem\*

Resolves noir-lang#4237 

## Summary\*
I replace is_pure() and has_side_effect() by can_be_replaced() and
can_eliminate_if_unused(), the former using
requires_acir_gen_predicate() which indicates if the instructions will
depend on the side_effect_enabled variable.


## Additional Context
I did not renamed has_side_effect() for Intrinsic because I believe the
name is accurate here.


## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

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

@@ -242,20 +242,11 @@ impl Instruction {
         matches!(self.result_type(), InstructionResultType::Unknown)
     }
 
-    /// Pure `Instructions` are instructions which have no side-effects and results are a function of the inputs only,
-    /// i.e. there are no interactions with memory.
-    ///
-    /// Pure instructions can be replaced with the results of another pure instruction with the same inputs.
-    pub(crate) fn is_pure(&self, dfg: &DataFlowGraph) -> bool {
+    /// Indicates if the instruction can be safely replaced with the results of another instruction with the same inputs.
+    pub(crate) fn can_be_deduplicated(&self, dfg: &DataFlowGraph) -> bool {
         use Instruction::*;
 
         match self {
-            Binary(bin) => {
-                // In ACIR, a division with a false predicate outputs (0,0), so it cannot replace another instruction unless they have the same predicate
-                bin.operator != BinaryOp::Div
-            }
-            Cast(_, _) | Truncate { .. } | Not(_) => true,
-
             // These either have side-effects or interact with memory
             Constrain(..)
             | EnableSideEffects { .. }
@@ -266,31 +257,36 @@ impl Instruction {
             | DecrementRc { .. }
             | RangeCheck { .. } => false,
 
-            // These can have different behavior depending on the EnableSideEffectsIf context.
-            // Enabling constant folding for these potentially enables replacing an enabled
-            // array get with one that was disabled. See
-            // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328.
-            ArrayGet { .. } | ArraySet { .. } => false,
-
             Call { func, .. } => match dfg[*func] {
                 Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(),
                 _ => false,
             },
+
+            // These can have different behavior depending on the EnableSideEffectsIf context.
+            // Replacing them with a similar instruction potentially enables replacing an instruction
+            // with one that was disabled. See
+            // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328.
+            Binary(_)
+            | Cast(_, _)
+            | Not(_)
+            | Truncate { .. }
+            | ArrayGet { .. }
+            | ArraySet { .. } => !self.requires_acir_gen_predicate(dfg),
         }
     }
 
-    pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool {
+    pub(crate) fn can_eliminate_if_unused(&self, dfg: &DataFlowGraph) -> bool {
         use Instruction::*;
         match self {
             Binary(binary) => {
                 if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) {
                     if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) {
-                        rhs == FieldElement::zero()
+                        rhs != FieldElement::zero()
                     } else {
-                        true
+                        false
                     }
                 } else {
-                    false
+                    true
                 }
             }
             Cast(_, _)
@@ -299,32 +295,65 @@ impl Instruction {
             | Allocate
             | Load { .. }
             | ArrayGet { .. }
-            | ArraySet { .. } => false,
+            | ArraySet { .. } => true,
 
             Constrain(..)
             | Store { .. }
             | EnableSideEffects { .. }
             | IncrementRc { .. }
             | DecrementRc { .. }
-            | RangeCheck { .. } => true,
+            | RangeCheck { .. } => false,
 
             // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
             Call { func, .. } => match dfg[*func] {
-                Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(),
+                Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(),
 
                 // All foreign functions are treated as having side effects.
                 // This is because they can be used to pass information
                 // from the ACVM to the external world during execution.
-                Value::ForeignFunction(_) => true,
+                Value::ForeignFunction(_) => false,
 
                 // We must assume that functions contain a side effect as we cannot inspect more deeply.
-                Value::Function(_) => true,
+                Value::Function(_) => false,
 
                 _ => false,
             },
         }
     }
 
+    /// If true the instruction will depends on enable_side_effects context during acir-gen
+    fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool {
+        match self {
+            Instruction::Binary(binary)
+                if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) =>
+            {
+                true
+            }
+            Instruction::EnableSideEffects { .. }
+            | Instruction::ArrayGet { .. }
+            | Instruction::ArraySet { .. } => true,
+
+            Instruction::Call { func, .. } => match dfg[*func] {
+                Value::Function(_) => true,
+                Value::Intrinsic(intrinsic) => {
+                    matches!(intrinsic, Intrinsic::SliceInsert | Intrinsic::SliceRemove)
+                }
+                _ => false,
+            },
+            Instruction::Cast(_, _)
+            | Instruction::Binary(_)
+            | Instruction::Not(_)
+            | Instruction::Truncate { .. }
+            | Instruction::Constrain(_, _, _)
+            | Instruction::RangeCheck { .. }
+            | Instruction::Allocate
+            | Instruction::Load { .. }
+            | Instruction::Store { .. }
+            | Instruction::IncrementRc { .. }
+            | Instruction::DecrementRc { .. } => false,
+        }
+    }
+
     /// Maps each ValueId inside this instruction to a new ValueId, returning the new instruction.
     /// Note that the returned instruction is fresh and will not have an assigned InstructionId
     /// until it is manually inserted in a DataFlowGraph later.

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

@@ -6,9 +6,8 @@
 //!   by the [`DataFlowGraph`] automatically as new instructions are pushed.
 //! - Check whether any input values have been constrained to be equal to a value of a simpler form
 //!   by a [constrain instruction][Instruction::Constrain]. If so, replace the input value with the simpler form.
-//! - Check whether the instruction is [pure][Instruction::is_pure()]
-//!   and there exists a duplicate instruction earlier in the same block.
-//!   If so, the instruction can be replaced with the results of this previous instruction.
+//! - Check whether the instruction [can_be_replaced][Instruction::can_be_replaced()]
+//!   by duplicate instruction earlier in the same block.
 //!
 //! These operations are done in parallel so that they can each benefit from each other
 //! without the need for multiple passes.
@@ -257,9 +256,9 @@ impl Context {
             }
         }
 
-        // If the instruction doesn't have side-effects, cache the results so we can reuse them if
-        // the same instruction appears again later in the block.
-        if instruction.is_pure(dfg) {
+        // If the instruction doesn't have side-effects and if it won't interact with enable_side_effects during acir_gen,
+        // we cache the results so we can reuse them if the same instruction appears again later in the block.
+        if instruction.can_be_deduplicated(dfg) {
             instruction_result_cache.insert(instruction, instruction_results);
         }
     }

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

@@ -108,12 +108,12 @@ impl Context {
     fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool {
         let instruction = &function.dfg[instruction_id];
 
-        if instruction.has_side_effects(&function.dfg) {
-            // If the instruction has side effects we should never remove it.
-            false
-        } else {
+        if instruction.can_eliminate_if_unused(&function.dfg) {
             let results = function.dfg.instruction_results(instruction_id);
             results.iter().all(|result| !self.used_values.contains(result))
+        } else {
+            // If the instruction has side effects we should never remove it.
+            false
         }
     }