diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index f56dfaa7c65..59abb349ddf 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -21,7 +21,7 @@ use super::{ dfg::{CallStack, DataFlowGraph}, function::Function, map::Id, - types::{NumericType, Type}, + types::Type, value::{Value, ValueId}, }; @@ -413,6 +413,7 @@ impl Instruction { Constrain(..) | EnableSideEffectsIf { .. } + | Store { .. } | IncrementRc { .. } | DecrementRc { .. } | RangeCheck { .. } => false, diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 487370488b9..668dd918b2b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -1,9 +1,9 @@ use acvm::{acir::AcirField, FieldElement}; use serde::{Deserialize, Serialize}; -use super::{ - DataFlowGraph, Instruction, InstructionResultType, NumericType, SimplifyResult, Type, ValueId, -}; +use crate::ssa::ir::types::NumericType; + +use super::{DataFlowGraph, Instruction, InstructionResultType, SimplifyResult, Type, ValueId}; /// Binary Operations allowed in the IR. /// Aside from the comparison operators (Eq and Lt), all operators diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/cast.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/cast.rs index ed588def1d7..c941a8eeeb0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/cast.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/cast.rs @@ -1,7 +1,9 @@ use acvm::{acir::AcirField, FieldElement}; use num_bigint::BigUint; -use super::{DataFlowGraph, Instruction, NumericType, SimplifyResult, Type, Value, ValueId}; +use crate::ssa::ir::types::NumericType; + +use super::{DataFlowGraph, Instruction, SimplifyResult, Type, Value, ValueId}; /// Try to simplify this cast instruction. If the instruction can be simplified to a known value, /// that value is returned. Otherwise None is returned. diff --git a/compiler/noirc_evaluator/src/ssa/ir/post_order.rs b/compiler/noirc_evaluator/src/ssa/ir/post_order.rs index 94ff96ba1d7..98cafea8be1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/post_order.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/post_order.rs @@ -49,7 +49,7 @@ impl PostOrder { stack.push((Visit::Last, block_id)); // Stack successors for visiting. Because items are taken from the top of the // stack, we push the item that's due for a visit first to the top. - for successor_id in func.dfg[block_id].successors().rev() { + for successor_id in func.dfg[block_id].successors() { if !visited.contains(&successor_id) { // This not visited check would also be cover by the next // iteration, but checking here two saves an iteration per successor. @@ -147,4 +147,6 @@ mod tests { let block_a_id = func.entry_block(); assert_eq!(post_order.0, [block_d_id, block_f_id, block_e_id, block_b_id, block_a_id]); } + + // TODO: Add `loop` test } diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index d8f33f6494d..cee3d9def44 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -7,6 +7,7 @@ use noirc_errors::Location; use crate::ssa::{ ir::{ basic_block::{BasicBlock, BasicBlockId}, + cfg::ControlFlowGraph, dfg::DataFlowGraph, function::Function, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, @@ -17,7 +18,10 @@ use crate::ssa::{ ssa_gen::{Ssa, SSA_WORD_SIZE}, }; -use super::rc::{pop_rc_for, RcInstruction}; +use super::{ + mem2reg::{AliasSet, StoreInstructionAliases}, + rc::{pop_rc_for, RcInstruction}, +}; impl Ssa { /// Performs Dead Instruction Elimination (DIE) to remove any instructions with @@ -25,61 +29,95 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn dead_instruction_elimination(mut self) -> Ssa { for function in self.functions.values_mut() { - function.dead_instruction_elimination(true); + Context::new(function).dead_instruction_elimination(function, true); } self } } -impl Function { +pub(crate) fn dce_with_store_aliases(function: &mut Function, aliases: StoreInstructionAliases) { + let mut context = Context::new(function); + context.store_aliases = Some(aliases); + context.dead_instruction_elimination(function, true); +} + +/// Per function context for tracking unused values and which instructions to remove. +struct Context { + used_values: HashSet, + instructions_to_remove: HashSet, + + /// IncrementRc & DecrementRc instructions must be revisited after the main DIE pass since + /// they technically contain side-effects but we still want to remove them if their + /// `value` parameter is not used elsewhere. + rc_instructions: Vec<(InstructionId, BasicBlockId)>, + + /// If we have information on which store instructions may alias other references we + /// can remove unused store instructions as well. This information is given by mem2reg + /// and is on each store instruction since which ValueIds are aliased would change over time + /// (so we cannot index by ValueId). + store_aliases: Option, + + cfg: ControlFlowGraph, + + visited_blocks: HashSet, +} + +impl Context { + fn new(function: &Function) -> Self { + Self { + used_values: HashSet::default(), + instructions_to_remove: HashSet::default(), + rc_instructions: Vec::new(), + store_aliases: None, + visited_blocks: HashSet::default(), + cfg: ControlFlowGraph::with_function(function), + } + } + /// Removes any unused instructions in the reachable blocks of the given function. /// /// The blocks of the function are iterated in post order, such that any blocks containing /// instructions that reference results from an instruction in another block are evaluated first. /// If we did not iterate blocks in this order we could not safely say whether or not the results /// of its instructions are needed elsewhere. - pub(crate) fn dead_instruction_elimination(&mut self, insert_out_of_bounds_checks: bool) { - let mut context = Context::default(); - for call_data in &self.dfg.data_bus.call_data { - context.mark_used_instruction_results(&self.dfg, call_data.array_id); + fn dead_instruction_elimination( + mut self, + function: &mut Function, + insert_out_of_bounds_checks: bool, + ) { + for call_data in &function.dfg.data_bus.call_data { + Self::mark_used_instruction_results( + &function.dfg, + call_data.array_id, + &mut self.used_values, + ); } let mut inserted_out_of_bounds_checks = false; - let blocks = PostOrder::with_function(self); + let blocks = PostOrder::with_function(function); for block in blocks.as_slice() { - inserted_out_of_bounds_checks |= context.remove_unused_instructions_in_block( - self, + inserted_out_of_bounds_checks |= self.remove_unused_instructions_in_block( + function, *block, insert_out_of_bounds_checks, ); + self.visited_blocks.insert(*block); } // If we inserted out of bounds check, let's run the pass again with those new // instructions (we don't want to remove those checks, or instructions that are // dependencies of those checks) if inserted_out_of_bounds_checks { - self.dead_instruction_elimination(false); - return; + let store_aliases = self.store_aliases; + self = Context::new(function); + self.store_aliases = store_aliases; + return self.dead_instruction_elimination(function, false); } - context.remove_rc_instructions(&mut self.dfg); + self.remove_rc_instructions(&mut function.dfg); } -} - -/// Per function context for tracking unused values and which instructions to remove. -#[derive(Default)] -struct Context { - used_values: HashSet, - instructions_to_remove: HashSet, - /// IncrementRc & DecrementRc instructions must be revisited after the main DIE pass since - /// they technically contain side-effects but we still want to remove them if their - /// `value` parameter is not used elsewhere. - rc_instructions: Vec<(InstructionId, BasicBlockId)>, -} - -impl Context { /// Steps backwards through the instruction of the given block, amassing a set of used values /// as it goes, and at the same time marking instructions for removal if they haven't appeared /// in the set thus far. @@ -106,25 +144,23 @@ impl Context { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); - 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(); - for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { + for (instruction_index, instruction_id) in block.instructions().iter().enumerate().rev() { let instruction = &function.dfg[*instruction_id]; - if self.is_unused(*instruction_id, function) { + if self.is_unused(*instruction_id, function, block_id) { self.instructions_to_remove.insert(*instruction_id); if insert_out_of_bounds_checks && instruction_might_result_in_out_of_bounds(function, instruction) { possible_index_out_of_bounds_indexes - .push(instructions_len - instruction_index - 1); + .push(instruction_index); } } else { use Instruction::*; @@ -132,8 +168,16 @@ impl Context { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { - self.mark_used_instruction_results(&function.dfg, value); + Self::mark_used_instruction_results( + &function.dfg, + value, + &mut self.used_values, + ); }); + + if matches!(instruction, Store { .. }) { + self.mark_used_store_aliases(&function.dfg, *instruction_id); + } } } @@ -169,12 +213,21 @@ impl Context { /// /// An instruction can be removed as long as it has no side-effects, and none of its result /// values have been referenced. - fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { + fn is_unused( + &self, + instruction_id: InstructionId, + function: &Function, + block: BasicBlockId, + ) -> bool { let instruction = &function.dfg[instruction_id]; if instruction.can_eliminate_if_unused(function) { let results = function.dfg.instruction_results(instruction_id); - results.iter().all(|result| !self.used_values.contains(result)) + results.iter().all(|result| { + !self.used_values.contains(result) + }) + } else if let Instruction::Store { address, .. } = instruction { + self.store_is_unused(instruction_id, *address, block) } else if let Instruction::Call { func, arguments } = instruction { // TODO: make this more general for instructions which don't have results but have side effects "sometimes" like `Intrinsic::AsWitness` let as_witness_id = function.dfg.get_intrinsic(Intrinsic::AsWitness); @@ -185,21 +238,56 @@ impl Context { } } + fn store_is_unused( + &self, + instruction_id: InstructionId, + address: ValueId, + block: BasicBlockId, + ) -> bool { + if !self.cfg.successors(block).all(|block| self.visited_blocks.contains(&block)) { + return false; + } + + if let Some(aliases) = Self::get_store_aliases(&self.store_aliases, instruction_id) { + aliases.any(|alias| self.used_values.contains(&alias)) == Some(false) + && !self.used_values.contains(&address) + } else { + false + } + } + /// Adds values referenced by the terminator to the set of used values. fn mark_terminator_values_as_used(&mut self, function: &Function, block: &BasicBlock) { block.unwrap_terminator().for_each_value(|value| { - self.mark_used_instruction_results(&function.dfg, value); + Self::mark_used_instruction_results(&function.dfg, value, &mut self.used_values); }); } /// Inspects a value and marks all instruction results as used. - fn mark_used_instruction_results(&mut self, dfg: &DataFlowGraph, value_id: ValueId) { + fn mark_used_instruction_results(dfg: &DataFlowGraph, value_id: ValueId, used_values: &mut HashSet) { let value_id = dfg.resolve(value_id); if matches!(&dfg[value_id], Value::Instruction { .. } | Value::Param { .. }) { - self.used_values.insert(value_id); + used_values.insert(value_id); } } + /// Marks each alias of `address` for the given store instruction as used + fn mark_used_store_aliases(&mut self, dfg: &DataFlowGraph, instruction: InstructionId) { + if let Some(aliases) = Self::get_store_aliases(&self.store_aliases, instruction) { + aliases.for_each(|alias| { + Self::mark_used_instruction_results(dfg, alias, &mut self.used_values) + }) + } + } + + // unknown stores mess this up... + fn get_store_aliases( + store_aliases: &Option, + instruction: InstructionId, + ) -> Option<&AliasSet> { + store_aliases.as_ref().and_then(|aliases| aliases.get(&instruction)) + } + fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) { for (rc, block) in self.rc_instructions { let value = match &dfg[rc] { @@ -282,11 +370,9 @@ impl Context { let call_stack = function.dfg.get_call_stack(instruction_id); - let (lhs, rhs) = if function.dfg.get_numeric_constant(*index).is_some() { + let lhs = if function.dfg.get_numeric_constant(*index).is_some() { // If we are here it means the index is known but out of bounds. That's always an error! - let false_const = function.dfg.make_constant(false.into(), Type::bool()); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (false_const, true_const) + function.dfg.make_constant(false.into(), Type::bool()) } else { // `index` will be relative to the flattened array length, so we need to take that into account let array_length = function.dfg.type_of_value(*array).flattened_size(); @@ -309,11 +395,10 @@ impl Context { None, call_stack.clone(), ); - let is_index_out_of_bounds = is_index_out_of_bounds.first(); - let true_const = function.dfg.make_constant(true.into(), Type::bool()); - (is_index_out_of_bounds, true_const) + is_index_out_of_bounds.first() }; + let rhs = function.dfg.make_constant(true.into(), Type::bool()); let (lhs, rhs) = apply_side_effects( side_effects_condition, lhs, diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index f91487fd73e..8ec923a5967 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -392,6 +392,7 @@ impl InlineContext { context.blocks.insert(context.source_function.entry_block(), entry_block); context.inline_blocks(ssa); + // translate databus values let databus = entry_point.dfg.data_bus.map_values(|t| context.translate_value(t)); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 77133d7d88d..f742409fce4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -82,7 +82,7 @@ use crate::ssa::{ ssa_gen::Ssa, }; -use self::alias_set::AliasSet; +pub(crate) use self::alias_set::AliasSet; use self::block::{Block, Expression}; impl Ssa { @@ -101,8 +101,8 @@ impl Function { pub(crate) fn mem2reg(&mut self) { let mut context = PerFunctionContext::new(self); context.mem2reg(); - context.remove_instructions(); context.update_data_bus(); + context.remove_instructions(); } } @@ -120,20 +120,11 @@ struct PerFunctionContext<'f> { /// from the middle of Vecs many times will be slower than a single call to `retain`. instructions_to_remove: HashSet, - /// Track a value's last load across all blocks. - /// If a value is not used in anymore loads we can remove the last store to that value. - last_loads: HashMap, - - /// Track whether a reference was passed into another entry point - /// This is needed to determine whether we can remove a store. - calls_reference_input: HashSet, - - /// Track whether a reference has been aliased, and store the respective - /// instruction that aliased that reference. - /// If that store has been set for removal, we can also remove this instruction. - aliased_references: HashMap>, + store_instruction_aliases: StoreInstructionAliases, } +pub(crate) type StoreInstructionAliases = HashMap; + impl<'f> PerFunctionContext<'f> { fn new(function: &'f mut Function) -> Self { let cfg = ControlFlowGraph::with_function(function); @@ -145,9 +136,7 @@ impl<'f> PerFunctionContext<'f> { inserter: FunctionInserter::new(function), blocks: BTreeMap::new(), instructions_to_remove: HashSet::default(), - last_loads: HashMap::default(), - calls_reference_input: HashSet::default(), - aliased_references: HashMap::default(), + store_instruction_aliases: HashMap::default(), } } @@ -173,85 +162,6 @@ impl<'f> PerFunctionContext<'f> { let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); terminator.for_each_value(|value| all_terminator_values.insert(value)); } - - // If we never load from an address within a function we can remove all stores to that address. - // This rule does not apply to reference parameters, which we must also check for before removing these stores. - for (_, block) in self.blocks.iter() { - for (store_address, store_instruction) in block.last_stores.iter() { - let store_alias_used = self.is_store_alias_used( - store_address, - block, - &all_terminator_values, - &per_func_block_params, - ); - - let is_dereference = block - .expressions - .get(store_address) - .map_or(false, |expression| matches!(expression, Expression::Dereference(_))); - - if self.last_loads.get(store_address).is_none() - && !store_alias_used - && !is_dereference - { - self.instructions_to_remove.insert(*store_instruction); - } - } - } - } - - // Extra checks on where a reference can be used aside a load instruction. - // Even if all loads to a reference have been removed we need to make sure that - // an allocation did not come from an entry point or was passed to an entry point. - fn is_store_alias_used( - &self, - store_address: &ValueId, - block: &Block, - all_terminator_values: &HashSet, - per_func_block_params: &HashSet, - ) -> bool { - let reference_parameters = self.reference_parameters(); - - if let Some(expression) = block.expressions.get(store_address) { - if let Some(aliases) = block.aliases.get(expression) { - let allocation_aliases_parameter = - aliases.any(|alias| reference_parameters.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - return true; - } - - let allocation_aliases_parameter = - aliases.any(|alias| per_func_block_params.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - return true; - } - - let allocation_aliases_parameter = - aliases.any(|alias| self.calls_reference_input.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - return true; - } - - let allocation_aliases_parameter = - aliases.any(|alias| all_terminator_values.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - return true; - } - - let allocation_aliases_parameter = aliases.any(|alias| { - if let Some(alias_instructions) = self.aliased_references.get(&alias) { - self.instructions_to_remove.is_disjoint(alias_instructions) - } else { - false - } - }); - if allocation_aliases_parameter == Some(true) { - return true; - } - } - } - - false } /// Collect the input parameters of the function which are of reference type. @@ -273,7 +183,7 @@ impl<'f> PerFunctionContext<'f> { if let Some(first_predecessor) = predecessors.next() { let mut first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); - first.last_stores.clear(); + first.clear_last_stores(); // Note that we have to start folding with the first block as the accumulator. // If we started with an empty block, an empty block union'd with any other block @@ -306,14 +216,6 @@ impl<'f> PerFunctionContext<'f> { } self.handle_terminator(block, &mut references); - - // If there's only 1 block in the function total, we can remove any remaining last stores - // as well. We can't do this if there are multiple blocks since subsequent blocks may - // reference these stores. - if self.post_order.as_slice().len() == 1 { - self.remove_stores_that_do_not_alias_parameters(&references); - } - self.blocks.insert(block, references); } @@ -328,53 +230,7 @@ impl<'f> PerFunctionContext<'f> { let mut aliases: HashMap = HashMap::default(); for param in params { - match dfg.type_of_value(*param) { - // If the type indirectly contains a reference we have to assume all references - // are unknown since we don't have any ValueIds to use. - Type::Reference(element) if element.contains_reference() => return, - Type::Reference(element) => { - let empty_aliases = AliasSet::known_empty(); - let alias_set = - aliases.entry(element.as_ref().clone()).or_insert(empty_aliases); - alias_set.insert(*param); - } - typ if typ.contains_reference() => return, - _ => continue, - } - } - - for aliases in aliases.into_values() { - let first = aliases.first(); - let first = first.expect("All parameters alias at least themselves or we early return"); - - let expression = Expression::Other(first); - let previous = references.aliases.insert(expression.clone(), aliases.clone()); - assert!(previous.is_none()); - - aliases.for_each(|alias| { - let previous = references.expressions.insert(alias, expression.clone()); - assert!(previous.is_none()); - }); - } - } - - /// Add all instructions in `last_stores` to `self.instructions_to_remove` which do not - /// possibly alias any parameters of the given function. - fn remove_stores_that_do_not_alias_parameters(&mut self, references: &Block) { - let reference_parameters = self.reference_parameters(); - - for (allocation, instruction) in &references.last_stores { - if let Some(expression) = references.expressions.get(allocation) { - if let Some(aliases) = references.aliases.get(expression) { - let allocation_aliases_parameter = - aliases.any(|alias| reference_parameters.contains(&alias)); - - // If `allocation_aliases_parameter` is known to be false - if allocation_aliases_parameter == Some(false) { - self.instructions_to_remove.insert(*instruction); - } - } - } + references.fresh_reference(*param); } } @@ -406,9 +262,8 @@ impl<'f> PerFunctionContext<'f> { self.inserter.map_value(result, value); self.instructions_to_remove.insert(instruction); } else { - references.mark_value_used(address, self.inserter.function); - - self.last_loads.insert(address, (instruction, block_id)); + references.mark_last_store_used(address); + // self.last_loads.insert(address, (instruction, block_id)); } } Instruction::Store { address, value } => { @@ -423,43 +278,49 @@ impl<'f> PerFunctionContext<'f> { // self.instructions_to_remove.insert(*last_store); // } - if self.inserter.function.dfg.value_is_reference(value) { - if let Some(expression) = references.expressions.get(&value) { - if let Some(aliases) = references.aliases.get(expression) { - aliases.for_each(|alias| { - self.aliased_references - .entry(alias) - .or_default() - .insert(instruction); - }); - } - } + // if self.inserter.function.dfg.value_is_reference(value) { + // if let Some(expression) = references.expressions.get(&value) { + // if let Some(aliases) = references.aliases.get(expression) { + // aliases.for_each(|alias| { + // self.aliased_references + // .entry(alias) + // .or_default() + // .insert(instruction); + // }); + // } + // } + // } + + // Remember the aliases this address has for DIE to remove later + let aliases = references.get_aliases_for_value(address); + if !aliases.is_unknown() { + self.store_instruction_aliases.insert(instruction, aliases.into_owned()); } + references.set_last_store(address, instruction, &mut self.instructions_to_remove); references.set_known_value(address, value); - references.last_stores.insert(address, instruction); } Instruction::Allocate => { // Register the new reference let result = self.inserter.function.dfg.instruction_results(instruction)[0]; - references.expressions.insert(result, Expression::Other(result)); - references.aliases.insert(Expression::Other(result), AliasSet::known(result)); + references.fresh_reference(result); } Instruction::ArrayGet { array, .. } => { let result = self.inserter.function.dfg.instruction_results(instruction)[0]; - references.mark_value_used(*array, self.inserter.function); + // references.mark_value_used(*array, self.inserter.function); if self.inserter.function.dfg.value_is_reference(result) { let array = self.inserter.function.dfg.resolve(*array); let expression = Expression::ArrayElement(Box::new(Expression::Other(array))); - - if let Some(aliases) = references.aliases.get_mut(&expression) { - aliases.insert(result); - } + references.add_expression_with_aliases( + result, + expression, + &AliasSet::known_empty(), + ); } } Instruction::ArraySet { array, value, .. } => { - references.mark_value_used(*array, self.inserter.function); + // references.mark_value_used(*array, self.inserter.function); let element_type = self.inserter.function.dfg.type_of_value(*value); if Self::contains_references(&element_type) { @@ -468,37 +329,22 @@ impl<'f> PerFunctionContext<'f> { let expression = Expression::ArrayElement(Box::new(Expression::Other(array))); - let mut aliases = if let Some(aliases) = references.aliases.get_mut(&expression) - { - aliases.clone() - } else if let Some((elements, _)) = - self.inserter.function.dfg.get_array_constant(array) - { - let aliases = references.collect_all_aliases(elements); - self.set_aliases(references, array, aliases.clone()); - aliases - } else { - AliasSet::unknown() - }; - - aliases.unify(&references.get_aliases_for_value(*value)); - - references.expressions.insert(result, expression.clone()); - references.aliases.insert(expression, aliases); + let element_aliases = self + .inserter + .function + .dfg + .get_array_constant(array) + .map(|(elements, _)| references.collect_all_aliases(elements)) + .unwrap_or_default(); + + references.add_expression_with_aliases( + result, + expression.clone(), + &element_aliases, + ); } } Instruction::Call { arguments, .. } => { - for arg in arguments { - if self.inserter.function.dfg.value_is_reference(*arg) { - if let Some(expression) = references.expressions.get(arg) { - if let Some(aliases) = references.aliases.get(expression) { - aliases.for_each(|alias| { - self.calls_reference_input.insert(alias); - }); - } - } - } - } self.mark_all_unknown(arguments, references); } Instruction::MakeArray { elements, typ } => { @@ -506,14 +352,10 @@ impl<'f> PerFunctionContext<'f> { // as a potential alias to the array itself. if Self::contains_references(typ) { let array = self.inserter.function.dfg.instruction_results(instruction)[0]; + let aliases = AliasSet::known_many(elements); let expr = Expression::ArrayElement(Box::new(Expression::Other(array))); - references.expressions.insert(array, expr.clone()); - let aliases = references.aliases.entry(expr).or_default(); - - for element in elements { - aliases.insert(*element); - } + references.add_expression_with_aliases(array, expr, &aliases); } } _ => (), @@ -531,33 +373,25 @@ impl<'f> PerFunctionContext<'f> { } } - fn set_aliases(&self, references: &mut Block, address: ValueId, new_aliases: AliasSet) { - let expression = - references.expressions.entry(address).or_insert(Expression::Other(address)); - let aliases = references.aliases.entry(expression.clone()).or_default(); - *aliases = new_aliases; - } - fn mark_all_unknown(&self, values: &[ValueId], references: &mut Block) { for value in values { if self.inserter.function.dfg.value_is_reference(*value) { let value = self.inserter.function.dfg.resolve(*value); references.set_unknown(value); - references.mark_value_used(value, self.inserter.function); + // references.mark_value_used(value, self.inserter.function); } } } - /// Remove any instructions in `self.instructions_to_remove` from the current function. - /// This is expected to contain any loads which were replaced and any stores which are - /// no longer needed. + /// Perform an alias-aware DIE pass to remove any unused stores and loads + /// + /// Issues: + /// 1. handling stores with unknown aliases + /// 2. die pass control-flow order on programs with loops fn remove_instructions(&mut self) { - // The order we iterate blocks in is not important - for block in self.post_order.as_slice() { - self.inserter.function.dfg[*block] - .instructions_mut() - .retain(|instruction| !self.instructions_to_remove.contains(instruction)); - } + let aliases = std::mem::take(&mut self.store_instruction_aliases); + super::die::dce_with_store_aliases(self.inserter.function, aliases); + eprintln!("SSA after dce_with_store_aliases:\n{}", self.inserter.function); } fn update_data_bus(&mut self) { @@ -578,13 +412,7 @@ impl<'f> PerFunctionContext<'f> { for (parameter, argument) in destination_parameters.iter().zip(arguments) { if self.inserter.function.dfg.value_is_reference(*parameter) { let argument = self.inserter.function.dfg.resolve(*argument); - - if let Some(expression) = references.expressions.get(&argument) { - if let Some(aliases) = references.aliases.get_mut(expression) { - // The argument reference is possibly aliased by this block parameter - aliases.insert(*parameter); - } - } + references.try_insert_alias(argument, *parameter); } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs index 4d768caa36b..c16a1c41d7b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs @@ -9,7 +9,7 @@ use crate::ssa::ir::value::ValueId; /// Note that we distinguish between "definitely has no aliases" - `Some(BTreeSet::new())`, and /// "unknown which aliases this may refer to" - `None`. #[derive(Debug, Default, Clone)] -pub(super) struct AliasSet { +pub(crate) struct AliasSet { aliases: Option>, } @@ -31,6 +31,10 @@ impl AliasSet { Self { aliases: Some(BTreeSet::new()) } } + pub(super) fn known_many(aliases: &im::Vector) -> AliasSet { + Self { aliases: Some(aliases.iter().copied().collect()) } + } + pub(super) fn is_unknown(&self) -> bool { self.aliases.is_none() } @@ -62,11 +66,11 @@ impl AliasSet { /// Returns `Some(true)` if `f` returns true for any known alias in this set. /// If this alias set is unknown, None is returned. - pub(super) fn any(&self, f: impl FnMut(ValueId) -> bool) -> Option { + pub(crate) fn any(&self, f: impl FnMut(ValueId) -> bool) -> Option { self.aliases.as_ref().map(|aliases| aliases.iter().copied().any(f)) } - pub(super) fn for_each(&self, mut f: impl FnMut(ValueId)) { + pub(crate) fn for_each(&self, mut f: impl FnMut(ValueId)) { if let Some(aliases) = &self.aliases { for alias in aliases { f(*alias); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index 532785d2928..0c76a30f136 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -1,10 +1,8 @@ use std::borrow::Cow; -use crate::ssa::ir::{ - function::Function, - instruction::{Instruction, InstructionId}, - value::ValueId, -}; +use fxhash::FxHashSet as HashSet; + +use crate::ssa::ir::{function::Function, instruction::InstructionId, value::ValueId}; use super::alias_set::AliasSet; @@ -19,21 +17,34 @@ pub(super) struct Block { /// Maps a ValueId to the Expression it represents. /// Multiple ValueIds can map to the same Expression, e.g. /// dereferences to the same allocation. - pub(super) expressions: im::OrdMap, + expressions: im::OrdMap, /// Each expression is tracked as to how many aliases it /// may have. If there is only 1, we can attempt to optimize /// out any known loads to that alias. Note that "alias" here /// includes the original reference as well. - pub(super) aliases: im::OrdMap, + aliases: im::OrdMap, /// Each allocate instruction result (and some reference block parameters) /// will map to a Reference value which tracks whether the last value stored /// to the reference is known. - pub(super) references: im::OrdMap, + /// + /// Note that this isn't intended to be accessed directly with any ValueId, + /// you should go through `expressions` then `aliases` first to find the + /// canonical ValueId if it exists. Doing so will ensure aliases in particular + /// are handled correctly. + references: im::OrdMap, - /// The last instance of a `Store` instruction to each address in this block - pub(super) last_stores: im::OrdMap, + /// The last Store instruction for a given canonical ValueId. + /// Invariant: if the last store is known, it can be removed when a new store is found. + /// Thus, it should also be cleared if there is ever a load that still requires the + /// last store. + /// + /// Note that this isn't intended to be accessed directly with any ValueId, + /// you should go through `expressions` then `aliases` first to find the + /// canonical ValueId if it exists. Doing so will ensure aliases in particular + /// are handled correctly. + last_stores: im::OrdMap, } /// An `Expression` here is used to represent a canonical key @@ -110,9 +121,33 @@ impl Block { } } + pub(super) fn add_expression_with_aliases( + &mut self, + result: ValueId, + expression: Expression, + aliases: &AliasSet, + ) { + let expr = self.expressions.entry(result).or_insert(expression); + self.aliases.entry(expr.clone()).or_insert(AliasSet::known(result)).unify(aliases); + } + + pub(super) fn try_insert_alias(&mut self, reference: ValueId, alias: ValueId) { + // FIXME: Should we invalidate all references if the expression is unknown? + if let Some(expr) = self.expressions.get(&reference) { + // FIXME: Should we invalidate all references if the alias set is unknown? + if let Some(aliases) = self.aliases.get_mut(expr) { + aliases.insert(alias); + } + } + } + + pub(super) fn fresh_reference(&mut self, result_address: ValueId) { + self.expressions.insert(result_address, Expression::Other(result_address)); + self.aliases.insert(Expression::Other(result_address), AliasSet::known(result_address)); + } + fn invalidate_all_references(&mut self) { self.references.clear(); - self.last_stores.clear(); } pub(super) fn unify(mut self, other: &Self) -> Self { @@ -141,7 +176,6 @@ impl Block { } } self.references = intersection; - self } @@ -181,41 +215,6 @@ impl Block { } } - fn keep_last_stores_for(&mut self, address: ValueId, function: &Function) { - let address = function.dfg.resolve(address); - self.keep_last_store(address, function); - self.for_each_alias_of(address, |t, alias| t.keep_last_store(alias, function)); - } - - fn keep_last_store(&mut self, address: ValueId, function: &Function) { - let address = function.dfg.resolve(address); - - if let Some(instruction) = self.last_stores.remove(&address) { - // Whenever we decide we want to keep a store instruction, we also need - // to go through its stored value and mark that used as well. - match &function.dfg[instruction] { - Instruction::Store { value, .. } => { - self.mark_value_used(*value, function); - } - other => { - unreachable!("last_store held an id of a non-store instruction: {other:?}") - } - } - } - } - - pub(super) fn mark_value_used(&mut self, value: ValueId, function: &Function) { - self.keep_last_stores_for(value, function); - - // We must do a recursive check for arrays since they're the only Values which may contain - // other ValueIds. - if let Some((array, _)) = function.dfg.get_array_constant(value) { - for value in array { - self.mark_value_used(value, function); - } - } - } - /// Collect all aliases used by the given value list pub(super) fn collect_all_aliases( &self, @@ -237,4 +236,43 @@ impl Block { Cow::Owned(AliasSet::unknown()) } + + pub(super) fn set_last_store( + &mut self, + address: ValueId, + instruction: InstructionId, + instructions_to_remove: &mut HashSet, + ) { + if let Some(expression) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expression) { + if let Some(address) = aliases.single_alias() { + if let Some(previous) = self.last_stores.insert(address, instruction) { + instructions_to_remove.insert(previous); + } + } + } + } + } + + pub(super) fn mark_last_store_used(&mut self, address: ValueId) { + if let Some(expression) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expression) { + if aliases.is_unknown() { + // uh-oh, we don't know at all what this reference refers to, could be anything. + // Now we have to mark every store as used + self.last_stores.clear(); + } else { + // More than one alias. We're not sure which it refers to so we have to + // conservatively invalidate all references it may refer to. + aliases.for_each(|alias| { + self.last_stores.remove(&alias); + }); + } + } + } + } + + pub(crate) fn clear_last_stores(&mut self) { + self.last_stores.clear(); + } }