diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 04f33d528cd..b6f11f45c07 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/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. diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 5a7134f3486..ac2f6424332 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/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); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index d1b3e1e83f5..d045762f9e9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/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 } }