diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index b0843f327c..148c9eafa2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -10,12 +10,14 @@ use crate::ssa::{ function::Function, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, - types::NumericType, + types::{NumericType, Type}, value::{Value, ValueId}, }, ssa_gen::Ssa, }; +use super::rc::{pop_rc_for, RcInstruction}; + impl Ssa { /// Performs Dead Instruction Elimination (DIE) to remove any instructions with /// unused results. @@ -104,6 +106,8 @@ impl Context { let instructions_len = block.instructions().len(); + let mut rc_tracker = RcTracker::default(); + // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. let mut possible_index_out_of_bounds_indexes = Vec::new(); @@ -131,8 +135,12 @@ impl Context { }); } } + + rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } + self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); + // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those // but leave the constrains (any any value needed by those constrains) @@ -517,6 +525,104 @@ fn apply_side_effects( (lhs, rhs) } +#[derive(Default)] +struct RcTracker { + // We can track IncrementRc instructions per block to determine whether they are useless. + // IncrementRc and DecrementRc instructions are normally side effectual instructions, but we remove + // them if their value is not used anywhere in the function. However, even when their value is used, their existence + // is pointless logic if there is no array set between the increment and the decrement of the reference counter. + // We track per block whether an IncrementRc instruction has a paired DecrementRc instruction + // with the same value but no array set in between. + // If we see an inc/dec RC pair within a block we can safely remove both instructions. + rcs_with_possible_pairs: HashMap>, + rc_pairs_to_remove: HashSet, + // We also separately track all IncrementRc instructions and all arrays which have been mutably borrowed. + // If an array has not been mutably borrowed we can then safely remove all IncrementRc instructions on that array. + inc_rcs: HashMap>, + mut_borrowed_arrays: HashSet, + // The SSA often creates patterns where after simplifications we end up with repeat + // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, + // and if the current instruction is also an IncrementRc on the same value we remove the current instruction. + // `None` if the previous instruction was anything other than an IncrementRc + previous_inc_rc: Option, +} + +impl RcTracker { + fn track_inc_rcs_to_remove(&mut self, instruction_id: InstructionId, function: &Function) { + let instruction = &function.dfg[instruction_id]; + + if let Instruction::IncrementRc { value } = instruction { + if let Some(previous_value) = self.previous_inc_rc { + if previous_value == *value { + self.rc_pairs_to_remove.insert(instruction_id); + } + } + self.previous_inc_rc = Some(*value); + } else { + self.previous_inc_rc = None; + } + + // DIE loops over a block in reverse order, so we insert an RC instruction for possible removal + // when we see a DecrementRc and check whether it was possibly mutated when we see an IncrementRc. + match instruction { + Instruction::IncrementRc { value } => { + if let Some(inc_rc) = + pop_rc_for(*value, function, &mut self.rcs_with_possible_pairs) + { + if !inc_rc.possibly_mutated { + self.rc_pairs_to_remove.insert(inc_rc.id); + self.rc_pairs_to_remove.insert(instruction_id); + } + } + + self.inc_rcs.entry(*value).or_default().insert(instruction_id); + } + Instruction::DecrementRc { value } => { + let typ = function.dfg.type_of_value(*value); + + // We assume arrays aren't mutated until we find an array_set + let dec_rc = + RcInstruction { id: instruction_id, array: *value, possibly_mutated: false }; + self.rcs_with_possible_pairs.entry(typ).or_default().push(dec_rc); + } + Instruction::ArraySet { array, .. } => { + let typ = function.dfg.type_of_value(*array); + if let Some(dec_rcs) = self.rcs_with_possible_pairs.get_mut(&typ) { + for dec_rc in dec_rcs { + dec_rc.possibly_mutated = true; + } + } + + self.mut_borrowed_arrays.insert(*array); + } + Instruction::Store { value, .. } => { + // We are very conservative and say that any store of an array value means it has the potential + // to be mutated. This is done due to the tracking of mutable borrows still being per block. + let typ = function.dfg.type_of_value(*value); + if matches!(&typ, Type::Array(..) | Type::Slice(..)) { + self.mut_borrowed_arrays.insert(*value); + } + } + _ => {} + } + } + + fn get_non_mutated_arrays(&self) -> HashSet { + self.inc_rcs + .keys() + .filter_map(|value| { + if !self.mut_borrowed_arrays.contains(value) { + Some(&self.inc_rcs[value]) + } else { + None + } + }) + .flatten() + .copied() + .collect() + } +} + #[cfg(test)] mod test { use std::sync::Arc;