From 78c1921e3a2035a14e4f2d190b5258f23116e85c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 22 Aug 2023 15:23:27 -0500 Subject: [PATCH 01/16] Add everything except alias handling --- .../noirc_evaluator/src/ssa/ir/basic_block.rs | 3 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 518 +++++++----------- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 8 +- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +- 4 files changed, 215 insertions(+), 319 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/basic_block.rs b/crates/noirc_evaluator/src/ssa/ir/basic_block.rs index 998591f7210..b3b566f8f37 100644 --- a/crates/noirc_evaluator/src/ssa/ir/basic_block.rs +++ b/crates/noirc_evaluator/src/ssa/ir/basic_block.rs @@ -122,10 +122,11 @@ impl BasicBlock { /// Return the jmp arguments, if any, of this block's TerminatorInstruction. /// - /// If this block has no terminator, or a Return terminator this will be empty. + /// If this block has no terminator, this will be empty. pub(crate) fn terminator_arguments(&self) -> &[ValueId] { match &self.terminator { Some(TerminatorInstruction::Jmp { arguments, .. }) => arguments, + Some(TerminatorInstruction::Return { return_values, .. }) => return_values, _ => &[], } } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index d83cda4a8b1..c9b5fed4237 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -1,69 +1,66 @@ //! mem2reg implements a pass for promoting values stored in memory to values in registers where //! possible. This is particularly important for converting our memory-based representation of //! mutable variables into values that are easier to manipulate. -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet}; use crate::ssa::{ ir::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, - dfg::DataFlowGraph, dom::DominatorTree, function::Function, - instruction::{Instruction, InstructionId, TerminatorInstruction}, + instruction::{Instruction, InstructionId}, post_order::PostOrder, - value::{Value, ValueId}, + value::ValueId, }, ssa_gen::Ssa, }; -use super::unrolling::{find_all_loops, Loops}; - impl Ssa { /// Attempts to remove any load instructions that recover values that are already available in /// scope, and attempts to remove stores that are subsequently redundant. /// As long as they are not stores on memory used inside of loops pub(crate) fn mem2reg(mut self) -> Ssa { for function in self.functions.values_mut() { - let mut all_protected_allocations = HashSet::new(); - let mut context = PerFunctionContext::new(function); - - for block in function.reachable_blocks() { - // Maps Load instruction id -> value to replace the result of the load with - let mut loads_to_substitute_per_block = BTreeMap::new(); - - // Maps Load result id -> value to replace the result of the load with - let mut load_values_to_substitute = BTreeMap::new(); - - let allocations_protected_by_block = context - .analyze_allocations_and_eliminate_known_loads( - &mut function.dfg, - &mut loads_to_substitute_per_block, - &mut load_values_to_substitute, - block, - ); - all_protected_allocations.extend(allocations_protected_by_block.into_iter()); - } - - for block in function.reachable_blocks() { - context.remove_unused_stores(&mut function.dfg, &all_protected_allocations, block); - } + context.mem2reg(function); + context.remove_instructions(function); } self } } struct PerFunctionContext { - last_stores_with_block: BTreeMap<(AllocId, BasicBlockId), ValueId>, - // Maps Load result id -> (value, block_id) - // Used to replace the result of a load with the appropriate block - load_values_to_substitute_per_func: BTreeMap, - store_ids: Vec, cfg: ControlFlowGraph, post_order: PostOrder, dom_tree: DominatorTree, - loops: Loops, + + blocks: BTreeMap, + + /// Load and Store instructions that should be removed at the end of the pass. + /// + /// We avoid removing individual instructions as we go since removing elements + /// from the middle of Vecs many times will be slower than a single call to `retain`. + instructions_to_remove: BTreeSet, +} + +#[derive(Default)] +struct Block { + /// The value of each reference at the end of this block + end_references: BTreeMap, +} + +#[derive(Clone, Default)] +struct Reference { + aliases: BTreeSet, + value: ReferenceValue, +} + +#[derive(Copy, Clone, PartialEq, Eq, Default)] +enum ReferenceValue { + #[default] + Unknown, + Known(ValueId), } impl PerFunctionContext { @@ -71,308 +68,194 @@ impl PerFunctionContext { let cfg = ControlFlowGraph::with_function(function); let post_order = PostOrder::with_function(function); let dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); + PerFunctionContext { - last_stores_with_block: BTreeMap::new(), - load_values_to_substitute_per_func: BTreeMap::new(), - store_ids: Vec::new(), cfg, post_order, dom_tree, - loops: find_all_loops(function), + blocks: BTreeMap::new(), + instructions_to_remove: BTreeSet::new(), } } -} -/// An AllocId is the ValueId returned from an allocate instruction. E.g. v0 in v0 = allocate. -/// This type alias is used to help signal where the only valid ValueIds are those that are from -/// an allocate instruction. -type AllocId = ValueId; + /// Apply the mem2reg pass to the given function. + /// + /// This function is expected to be the same one that the internal cfg, post_order, and + /// dom_tree were created from. + fn mem2reg(&mut self, function: &mut Function) { + // Iterate each block in reverse post order = forward order + let mut block_order = PostOrder::with_function(function).into_vec(); + block_order.reverse(); + + for block in block_order { + let references = self.find_starting_references(block); + self.analyze_block(function, block, references); + } + } -impl PerFunctionContext { - // Attempts to remove load instructions for which the result is already known from previous - // store instructions to the same address in the same block. - fn analyze_allocations_and_eliminate_known_loads( + /// The value of each reference at the start of the given block is the unification + /// of the value of the same reference at the end of its predecessor blocks. + fn find_starting_references(&self, block: BasicBlockId) -> Block { + self.cfg + .predecessors(block) + .fold(Block::default(), |block, predecessor| block.unify(&self.blocks[&predecessor])) + } + + /// Analyze a block with the given starting reference values. + /// + /// This will remove any known loads in the block and track the value of references + /// as they are stored to. When this function is finished, the value of each reference + /// at the end of this block will be remembered in `self.blocks`. + fn analyze_block( &mut self, - dfg: &mut DataFlowGraph, - loads_to_substitute: &mut BTreeMap, - load_values_to_substitute_per_block: &mut BTreeMap, - block_id: BasicBlockId, - ) -> HashSet { - let mut protected_allocations = HashSet::new(); - let block = &dfg[block_id]; - - // Check whether the block has a common successor here to avoid analyzing - // the CFG for every block instruction. - let has_common_successor = self.has_common_successor(block_id); - - for instruction_id in block.instructions() { - match &dfg[*instruction_id] { - Instruction::Store { mut address, value } => { - if has_common_successor { - protected_allocations.insert(address); - } - address = self.fetch_load_value_to_substitute(block_id, address); - - self.last_stores_with_block.insert((address, block_id), *value); - self.store_ids.push(*instruction_id); - - if has_common_successor { - protected_allocations.insert(address); - } - } - Instruction::Load { mut address } => { - address = self.fetch_load_value_to_substitute(block_id, address); - - let found_last_value = self.find_load_to_substitute( - block_id, - address, - dfg, - instruction_id, - loads_to_substitute, - load_values_to_substitute_per_block, - ); - if !found_last_value { - // We want to protect allocations that do not have a load to substitute - protected_allocations.insert(address); - // We also want to check for allocations that share a value - // with the one we are protecting. - // This check prevents the pass from removing stores to a value that - // is used by reference aliases in different blocks - protected_allocations.insert(dfg.resolve(address)); - } - } - Instruction::Call { arguments, .. } => { - for arg in arguments { - if Self::value_is_from_allocation(*arg, dfg) { - protected_allocations.insert(*arg); - } - } - } - _ => { - // Nothing to do - } - } - } + function: &mut Function, + block: BasicBlockId, + mut references: Block, + ) { + // TODO: Can we avoid cloning here? + let instructions = function.dfg[block].instructions().to_vec(); + let mut last_stores = BTreeMap::new(); - // Identify any arrays that are returned from this function - if let TerminatorInstruction::Return { return_values } = block.unwrap_terminator() { - for value in return_values { - if Self::value_is_from_allocation(*value, dfg) { - protected_allocations.insert(*value); - } - } + for instruction in instructions { + self.analyze_instruction(function, &mut references, instruction, &mut last_stores); } - // Substitute load result values - for (result_value, new_value) in load_values_to_substitute_per_block { - let result_value = dfg.resolve(*result_value); - dfg.set_value_from_id(result_value, *new_value); - } + let terminator_arguments = function.dfg[block].terminator_arguments(); + self.mark_all_unknown(terminator_arguments, &mut references, &mut last_stores); - // Delete load instructions - // Even though we could let DIE handle this, doing it here makes the debug output easier - // to read. - dfg[block_id] - .instructions_mut() - .retain(|instruction| !loads_to_substitute.contains_key(instruction)); + // 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 { + for (_, instruction) in last_stores { + self.instructions_to_remove.insert(instruction); + } + } - protected_allocations + self.blocks.insert(block, references); } - // This method will fetch already saved load values to substitute for a given address. - // The search starts at the block supplied as a parameter. If there is not a load to substitute - // the CFG is analyzed to determine whether a predecessor block has a load value to substitute. - // If there is no load value to substitute the original address is returned. - fn fetch_load_value_to_substitute( + fn analyze_instruction( &mut self, - block_id: BasicBlockId, - address: ValueId, - ) -> ValueId { - let mut stack = vec![block_id]; - let mut visited = HashSet::new(); - - while let Some(block) = stack.pop() { - visited.insert(block); - - // We do not want to substitute loads that take place within loops or yet to be simplified branches - // as this pass can occur before loop unrolling and flattening. - // The mem2reg pass should be ran again following all optimization passes as new values - // may be able to be promoted - for l in self.loops.yet_to_unroll.iter() { - // We do not want to substitute loads that take place within loops as this pass - // can occur before loop unrolling - // The pass should be ran again following loop unrolling as new values - if l.blocks.contains(&block) { - return address; + function: &mut Function, + references: &mut Block, + instruction: InstructionId, + last_stores: &mut BTreeMap, + ) { + match &function.dfg[instruction] { + Instruction::Load { address } => { + // If the load is known, replace it with the known value and remove the load + if let Some(value) = references.get_known_value(*address) { + let result = function.dfg.instruction_results(instruction)[0]; + function.dfg.set_value_from_id(result, value); + + self.instructions_to_remove.insert(instruction); + } else { + last_stores.remove(address); } } - - // Check whether there is a load value to substitute in the current block. - // Return the value if found. - if let Some((value, load_block_id)) = - self.load_values_to_substitute_per_func.get(&address) - { - if *load_block_id == block { - return *value; + Instruction::Store { address, value } => { + // If there was another store to this instruction without any (unremoved) loads or + // function calls in-between, we can remove the previous store. + if let Some(last_store) = last_stores.get(address) { + self.instructions_to_remove.insert(*last_store); } - } - // If no load values to substitute have been found in the current block, check the block's predecessors. - if let Some(predecessor) = self.block_has_predecessor(block, &visited) { - stack.push(predecessor); + references.set_known_value(*address, *value); + last_stores.insert(*address, instruction); } + Instruction::Call { arguments, .. } => { + self.mark_all_unknown(arguments, references, last_stores); + } + + // TODO: Track aliases here + // Instruction::ArrayGet { array, index } => todo!(), + // Instruction::ArraySet { array, index, value } => todo!(), + // We can ignore Allocate since the value is still unknown until the first Store + _ => (), } - address } - // This method determines which loads should be substituted and saves them - // to be substituted further in the pass. - // Starting at the block supplied as a parameter, we check whether a store has occurred with the given address. - // If no store has occurred in the supplied block, the CFG is analyzed to determine whether - // a predecessor block has a store at the given address. - fn find_load_to_substitute( - &mut self, - block_id: BasicBlockId, - address: ValueId, - dfg: &DataFlowGraph, - instruction_id: &InstructionId, - loads_to_substitute: &mut BTreeMap, - load_values_to_substitute_per_block: &mut BTreeMap, - ) -> bool { - let mut stack = vec![block_id]; - let mut visited = HashSet::new(); - - while let Some(block) = stack.pop() { - visited.insert(block); - - // We do not want to substitute loads that take place within loops or yet to be simplified branches - // as this pass can occur before loop unrolling and flattening. - // The mem2reg pass should be ran again following all optimization passes as new values - // may be able to be promoted - for l in self.loops.yet_to_unroll.iter() { - // We do not want to substitute loads that take place within loops as this pass - // can occur before loop unrolling - // The pass should be ran again following loop unrolling as new values - if l.blocks.contains(&block) { - return false; - } - } + fn mark_all_unknown( + &self, + values: &[ValueId], + references: &mut Block, + last_stores: &mut BTreeMap, + ) { + for value in values { + references.set_unknown(*value); - // Check whether there has been a store instruction in the current block - // If there has been a store, add a load to be substituted. - if let Some(last_value) = self.last_stores_with_block.get(&(address, block)) { - let result_value = *dfg - .instruction_results(*instruction_id) - .first() - .expect("ICE: Load instructions should have single result"); - - loads_to_substitute.insert(*instruction_id, *last_value); - load_values_to_substitute_per_block.insert(result_value, *last_value); - self.load_values_to_substitute_per_func.insert(result_value, (*last_value, block)); - return true; - } + println!("Removing {value}"); + last_stores.remove(value); + } + } - // If no load values to substitute have been found in the current block, check the block's predecessors. - if let Some(predecessor) = self.block_has_predecessor(block, &visited) { - stack.push(predecessor); - } + /// 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. + fn remove_instructions(&self, function: &mut Function) { + // The order we iterate blocks in is not important + for block in self.post_order.as_slice() { + function.dfg[*block] + .instructions_mut() + .retain(|instruction| !self.instructions_to_remove.contains(instruction)); } - false } +} - // If no loads or stores have been found in the current block, check the block's predecessors. - // Only check blocks with one predecessor as otherwise a constant value could be propogated - // through successor blocks with multiple branches that rely on other simplification passes - // such as loop unrolling or flattening of the CFG. - fn block_has_predecessor( - &mut self, - block_id: BasicBlockId, - visited: &HashSet, - ) -> Option { - let mut predecessors = self.cfg.predecessors(block_id); - if predecessors.len() == 1 { - let predecessor = predecessors.next().unwrap(); - if self.dom_tree.is_reachable(predecessor) - && self.dom_tree.dominates(predecessor, block_id) - && !visited.contains(&predecessor) - { - return Some(predecessor); +impl Block { + /// If the given reference id points to a known value, return the value + fn get_known_value(&self, address: ValueId) -> Option { + if let Some(reference) = self.end_references.get(&address) { + if let ReferenceValue::Known(value) = reference.value { + return Some(value); } } None } - fn has_common_successor(&mut self, block_id: BasicBlockId) -> bool { - let mut predecessors = self.cfg.predecessors(block_id); - if let Some(predecessor) = predecessors.next() { - let pred_successors = self.find_all_successors(predecessor); - let current_successors: HashSet<_> = self.cfg.successors(block_id).collect(); - return pred_successors.into_iter().any(|b| current_successors.contains(&b)); - } - false + /// If the given address is known, set its value to `ReferenceValue::Known(value)`. + fn set_known_value(&mut self, address: ValueId, value: ValueId) { + // TODO: Aliases + let entry = self.end_references.entry(address).or_default(); + entry.value = ReferenceValue::Known(value); } - fn find_all_successors(&self, block_id: BasicBlockId) -> HashSet { - let mut stack = vec![]; - let mut visited = HashSet::new(); + fn set_unknown(&mut self, address: ValueId) { + self.end_references.entry(address).and_modify(|reference| { + reference.value = ReferenceValue::Unknown; + }); + } - // Fetch initial block successors - let successors = self.cfg.successors(block_id); - for successor in successors { - if !visited.contains(&successor) { - stack.push(successor); - } - } + fn unify(mut self, other: &Self) -> Self { + for (reference_id, other_reference) in &other.end_references { + let new_reference = if let Some(old_reference) = self.end_references.get(reference_id) { + old_reference.unify(other_reference) + } else { + other_reference.clone() + }; - // Follow the CFG to fetch the remaining successors - while let Some(block) = stack.pop() { - visited.insert(block); - let successors = self.cfg.successors(block); - for successor in successors { - if !visited.contains(&successor) { - stack.push(successor); - } - } + self.end_references.insert(*reference_id, new_reference); } - visited + self } +} - /// Checks whether the given value id refers to an allocation. - fn value_is_from_allocation(value: ValueId, dfg: &DataFlowGraph) -> bool { - match &dfg[value] { - Value::Instruction { instruction, .. } => { - matches!(&dfg[*instruction], Instruction::Allocate) - } - _ => false, - } +impl Reference { + fn unify(&self, other: &Self) -> Self { + let value = self.value.unify(other.value); + let aliases = self.aliases.union(&other.aliases).copied().collect(); + Self { value, aliases } } +} - /// Removes all store instructions identified during analysis that aren't present in the - /// provided `protected_allocations` `HashSet`. - fn remove_unused_stores( - &self, - dfg: &mut DataFlowGraph, - protected_allocations: &HashSet, - block_id: BasicBlockId, - ) { - // Scan for unused stores - let mut stores_to_remove = HashSet::new(); - - for instruction_id in &self.store_ids { - let address = match &dfg[*instruction_id] { - Instruction::Store { address, .. } => *address, - _ => unreachable!("store_ids should contain only store instructions"), - }; - - if !protected_allocations.contains(&address) { - stores_to_remove.insert(*instruction_id); - } +impl ReferenceValue { + fn unify(self, other: Self) -> Self { + if self == other { + self + } else { + ReferenceValue::Unknown } - - // Delete unused stores - dfg[block_id] - .instructions_mut() - .retain(|instruction| !stores_to_remove.contains(instruction)); } } @@ -423,8 +306,6 @@ mod tests { let ssa = builder.finish().mem2reg().fold_constants(); - println!("{ssa}"); - let func = ssa.main(); let block_id = func.entry_block(); @@ -495,7 +376,9 @@ mod tests { let func = ssa.main(); let block_id = func.entry_block(); - // Store affects outcome of returned array, and can't be removed + println!("{ssa}"); + + // Store is needed by the return value, and can't be removed assert_eq!(count_stores(block_id, &func.dfg), 1); let ret_val_id = match func.dfg[block_id].terminator().unwrap() { @@ -562,11 +445,13 @@ mod tests { assert_eq!(ssa.main().reachable_blocks().len(), 2); // Expected result: - // fn main { + // acir fn main f0 { // b0(): // v0 = allocate + // store Field 5 at v0 // jmp b1(Field 5) // b1(v3: Field): + // store Field 6 at v0 // return v3, Field 5, Field 6 // Optimized to constants 5 and 6 // } let ssa = ssa.mem2reg(); @@ -578,9 +463,9 @@ mod tests { assert_eq!(count_loads(main.entry_block(), &main.dfg), 0); assert_eq!(count_loads(b1, &main.dfg), 0); - // All stores should be removed - assert_eq!(count_stores(main.entry_block(), &main.dfg), 0); - assert_eq!(count_stores(b1, &main.dfg), 0); + // Neither store is removed since they are each the last in the block and there are multiple blocks + assert_eq!(count_stores(main.entry_block(), &main.dfg), 1); + assert_eq!(count_stores(b1, &main.dfg), 1); // The jmp to b1 should also be a constant 5 now match main.dfg[main.entry_block()].terminator() { @@ -597,7 +482,7 @@ mod tests { // Test that a load in a predecessor block has been removed if the value // is later stored in a successor block #[test] - fn store_with_load_in_predecessor_block() { + fn load_aliases_in_predecessor_block() { // fn main { // b0(): // v0 = allocate @@ -647,14 +532,18 @@ mod tests { assert_eq!(ssa.main().reachable_blocks().len(), 2); // Expected result: - // fn main { - // b0(): - // v0 = allocate - // v2 = allocate - // jmp b1() - // b1(): - // v8 = eq Field 2, Field 2 - // return + // acir fn main f0 { + // b0(): + // v0 = allocate + // store Field 0 at v0 + // v2 = allocate + // store v0 at v2 + // jmp b1() + // b1(): + // store Field 1 at v0 + // store Field 2 at v0 + // v8 = eq Field 1, Field 2 + // return // } let ssa = ssa.mem2reg(); @@ -665,13 +554,16 @@ mod tests { assert_eq!(count_loads(main.entry_block(), &main.dfg), 0); assert_eq!(count_loads(b1, &main.dfg), 0); - // All stores should be removed - assert_eq!(count_stores(main.entry_block(), &main.dfg), 0); - assert_eq!(count_stores(b1, &main.dfg), 0); + println!("{ssa}"); + + // The stores are not removed in this case + assert_eq!(count_stores(main.entry_block(), &main.dfg), 2); + assert_eq!(count_stores(b1, &main.dfg), 2); let b1_instructions = main.dfg[b1].instructions(); - // The first instruction should be a binary operation - match &main.dfg[b1_instructions[0]] { + + // The last instruction in b1 should be a binary operation + match &main.dfg[*b1_instructions.last().unwrap()] { Instruction::Binary(binary) => { let lhs = main.dfg.get_numeric_constant(binary.lhs).expect("Expected constant value"); diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index 9bf62bda1cb..f9c92f8335e 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -44,7 +44,7 @@ impl Ssa { } } -pub(crate) struct Loop { +struct Loop { /// The header block of a loop is the block which dominates all the /// other blocks in the loop. header: BasicBlockId, @@ -57,12 +57,12 @@ pub(crate) struct Loop { pub(crate) blocks: HashSet, } -pub(crate) struct Loops { +struct Loops { /// The loops that failed to be unrolled so that we do not try to unroll them again. /// Each loop is identified by its header block id. failed_to_unroll: HashSet, - pub(crate) yet_to_unroll: Vec, + yet_to_unroll: Vec, modified_blocks: HashSet, cfg: ControlFlowGraph, dom_tree: DominatorTree, @@ -70,7 +70,7 @@ pub(crate) struct Loops { /// Find a loop in the program by finding a node that dominates any predecessor node. /// The edge where this happens will be the back-edge of the loop. -pub(crate) fn find_all_loops(function: &Function) -> Loops { +fn find_all_loops(function: &Function) -> Loops { let cfg = ControlFlowGraph::with_function(function); let post_order = PostOrder::with_function(function); let mut dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index cc3a7c02a75..1367b1753af 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -130,7 +130,10 @@ impl<'a> FunctionContext<'a> { let slice_contents = self.codegen_array(elements, typ[1].clone()); Tree::Branch(vec![slice_length.into(), slice_contents]) } - _ => unreachable!("ICE: array literal type must be an array or a slice, but got {}", array.typ), + _ => unreachable!( + "ICE: array literal type must be an array or a slice, but got {}", + array.typ + ), } } ast::Literal::Integer(value, typ) => { From 49cc0c819bfc63c8e688e5aac6787a15e2c519f0 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 23 Aug 2023 11:51:53 -0500 Subject: [PATCH 02/16] Fix all tests --- .../str_as_bytes/src/main.nr | 22 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 230 ++++++++++++++---- 2 files changed, 197 insertions(+), 55 deletions(-) diff --git a/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr b/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr index 6890b156b15..80a8ccbd77a 100644 --- a/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr +++ b/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr @@ -1,18 +1,18 @@ use dep::std; fn main() { let a = "hello"; - let b = a.as_bytes(); - assert(b[0]==104); - assert(b[1]==101); - assert(b[2]==108); - assert(b[3]==108); - assert(b[4]==111); - assert(b.len()==5); + // let b = a.as_bytes(); + // assert(b[0]==104); + // assert(b[1]==101); + // assert(b[2]==108); + // assert(b[3]==108); + // assert(b[4]==111); + // assert(b.len()==5); let mut c = a.as_bytes_vec(); assert(c.get(0)==104); - assert(c.get(1)==101); - assert(c.get(2)==108); - assert(c.get(3)==108); - assert(c.get(4)==111); + // assert(c.get(1)==101); + // assert(c.get(2)==108); + // assert(c.get(3)==108); + // assert(c.get(4)==111); assert(c.len()==5); } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index c9b5fed4237..e7219b05912 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -11,6 +11,7 @@ use crate::ssa::{ function::Function, instruction::{Instruction, InstructionId}, post_order::PostOrder, + types::Type, value::ValueId, }, ssa_gen::Ssa, @@ -44,21 +45,29 @@ struct PerFunctionContext { instructions_to_remove: BTreeSet, } -#[derive(Default)] +#[derive(Debug, Default, Clone)] struct Block { - /// The value of each reference at the end of this block - end_references: BTreeMap, + expressions: BTreeMap, + + aliases: BTreeMap>, + + alias_sets: BTreeMap, +} + +#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] +enum Expression { + Dereference(Box), + Other(ValueId), } -#[derive(Clone, Default)] +#[derive(Debug, Clone)] struct Reference { aliases: BTreeSet, value: ReferenceValue, } -#[derive(Copy, Clone, PartialEq, Eq, Default)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] enum ReferenceValue { - #[default] Unknown, Known(ValueId), } @@ -95,10 +104,26 @@ impl PerFunctionContext { /// The value of each reference at the start of the given block is the unification /// of the value of the same reference at the end of its predecessor blocks. - fn find_starting_references(&self, block: BasicBlockId) -> Block { - self.cfg - .predecessors(block) - .fold(Block::default(), |block, predecessor| block.unify(&self.blocks[&predecessor])) + fn find_starting_references(&mut self, block: BasicBlockId) -> Block { + let mut predecessors = self.cfg.predecessors(block); + println!( + "find_starting_references block {block} with {} predecessor(s)", + predecessors.len() + ); + + if let Some(first_predecessor) = predecessors.next() { + let first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); + + // 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 + // is always also empty so we'd never be able to track any references across blocks. + predecessors.fold(first, |block, predecessor| { + let predecessor = self.blocks.entry(predecessor).or_default(); + block.unify(predecessor) + }) + } else { + Block::default() + } } /// Analyze a block with the given starting reference values. @@ -121,7 +146,7 @@ impl PerFunctionContext { } let terminator_arguments = function.dfg[block].terminator_arguments(); - self.mark_all_unknown(terminator_arguments, &mut references, &mut last_stores); + self.mark_all_unknown(terminator_arguments, function, &mut references, &mut last_stores); // 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 @@ -144,34 +169,50 @@ impl PerFunctionContext { ) { match &function.dfg[instruction] { Instruction::Load { address } => { + let address = function.dfg.resolve(*address); + + let result = function.dfg.instruction_results(instruction)[0]; + references.remember_dereference(function, address, result); + // If the load is known, replace it with the known value and remove the load - if let Some(value) = references.get_known_value(*address) { + if let Some(value) = references.get_known_value(address) { let result = function.dfg.instruction_results(instruction)[0]; function.dfg.set_value_from_id(result, value); self.instructions_to_remove.insert(instruction); } else { - last_stores.remove(address); + last_stores.remove(&address); } } Instruction::Store { address, value } => { + let address = function.dfg.resolve(*address); + let value = function.dfg.resolve(*value); + // If there was another store to this instruction without any (unremoved) loads or // function calls in-between, we can remove the previous store. - if let Some(last_store) = last_stores.get(address) { + if let Some(last_store) = last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); } - references.set_known_value(*address, *value); - last_stores.insert(*address, instruction); + references.set_known_value(address, value); + last_stores.insert(address, instruction); } Instruction::Call { arguments, .. } => { - self.mark_all_unknown(arguments, references, last_stores); + self.mark_all_unknown(arguments, function, references, last_stores); + } + Instruction::Allocate => { + // Register the new reference + let result = function.dfg.instruction_results(instruction)[0]; + references.expressions.insert(result, Expression::Other(result)); + + let mut aliases = BTreeSet::new(); + aliases.insert(result); + references.aliases.insert(Expression::Other(result), aliases); } // TODO: Track aliases here // Instruction::ArrayGet { array, index } => todo!(), // Instruction::ArraySet { array, index, value } => todo!(), - // We can ignore Allocate since the value is still unknown until the first Store _ => (), } } @@ -179,14 +220,16 @@ impl PerFunctionContext { fn mark_all_unknown( &self, values: &[ValueId], + function: &Function, references: &mut Block, last_stores: &mut BTreeMap, ) { for value in values { - references.set_unknown(*value); - - println!("Removing {value}"); - last_stores.remove(value); + if function.dfg.type_of_value(*value) == Type::Reference { + let value = function.dfg.resolve(*value); + references.set_unknown(value); + last_stores.remove(&value); + } } } @@ -206,39 +249,137 @@ impl PerFunctionContext { impl Block { /// If the given reference id points to a known value, return the value fn get_known_value(&self, address: ValueId) -> Option { - if let Some(reference) = self.end_references.get(&address) { - if let ReferenceValue::Known(value) = reference.value { - return Some(value); + if let Some(expression) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expression) { + // We could allow multiple aliases if we check that the reference + // value in each is equal. + if aliases.len() == 1 { + let alias = aliases.first().expect("There should be exactly 1 alias"); + + if let Some(reference) = self.alias_sets.get(alias) { + if let ReferenceValue::Known(value) = reference.value { + println!("get_known_value: returning {value} for alias {alias}"); + return Some(value); + } else { + println!("get_known_value: ReferenceValue::Unknown for alias {alias}"); + } + } else { + println!("get_known_value: No alias_set for alias {alias}"); + } + } else { + println!("get_known_value: {} aliases for address {address}", aliases.len()); + } + } else { + println!("get_known_value: No known aliases for address {address}"); } + } else { + println!("get_known_value: No expression for address {address}"); } None } /// If the given address is known, set its value to `ReferenceValue::Known(value)`. fn set_known_value(&mut self, address: ValueId, value: ValueId) { - // TODO: Aliases - let entry = self.end_references.entry(address).or_default(); - entry.value = ReferenceValue::Known(value); + self.set_value(address, ReferenceValue::Known(value)); } fn set_unknown(&mut self, address: ValueId) { - self.end_references.entry(address).and_modify(|reference| { - reference.value = ReferenceValue::Unknown; - }); + self.set_value(address, ReferenceValue::Unknown); + } + + fn set_value(&mut self, address: ValueId, value: ReferenceValue) { + // TODO: Verify this does not fail in the case of reference parameters + if let Some(expression) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expression) { + if aliases.is_empty() { + // uh-oh, we don't know at all what this reference refers to, could be anything. + // Now we have to invalidate every reference we know of + todo!("empty alias set"); + } else if aliases.len() == 1 { + let alias = aliases.first().expect("There should be exactly 1 alias"); + + if let Some(reference) = self.alias_sets.get_mut(alias) { + println!("set_known_value: Set value to {value:?} for alias {alias}"); + reference.value = value; + } else { + let reference = Reference { value, aliases: BTreeSet::new() }; + self.alias_sets.insert(*alias, reference); + println!("set_known_value: Created new alias set for alias {alias}, with value {value:?}"); + } + } else { + println!("set_known_value: {} aliases for expression {expression:?}, marking all unknown", aliases.len()); + // More than one alias. We're not sure which it refers to so we have to + // conservatively invalidate all references it may refer to. + for alias in aliases.iter() { + if let Some(reference) = self.alias_sets.get_mut(alias) { + println!(" Marking {alias} unknown"); + reference.value = ReferenceValue::Unknown; + } else { + println!(" {alias} already unknown"); + } + } + } + } else { + println!("set_known_value: ReferenceValue::Unknown for expression {expression:?}"); + } + } else { + println!("\n\n!! set_known_value: Creating fresh expression for {address} in set_value... aliases will be empty\n\n"); + self.expressions.insert(address, Expression::Other(address)); + } } fn unify(mut self, other: &Self) -> Self { - for (reference_id, other_reference) in &other.end_references { - let new_reference = if let Some(old_reference) = self.end_references.get(reference_id) { - old_reference.unify(other_reference) + println!("unify:\n {self:?}\n {other:?}"); + + for (value_id, expression) in &other.expressions { + if let Some(existing) = self.expressions.get(value_id) { + assert_eq!(existing, expression, "Expected expressions for {value_id} to be equal"); } else { - other_reference.clone() - }; + self.expressions.insert(*value_id, expression.clone()); + } + } + + for (expression, new_aliases) in &other.aliases { + let expression = expression.clone(); + + self.aliases + .entry(expression) + .and_modify(|aliases| { + for alias in new_aliases { + aliases.insert(*alias); + } + }) + .or_insert_with(|| new_aliases.clone()); + } - self.end_references.insert(*reference_id, new_reference); + // Keep only the references present in both maps. + let mut intersection = BTreeMap::new(); + for (value_id, reference) in &other.alias_sets { + if let Some(existing) = self.alias_sets.get(value_id) { + intersection.insert(*value_id, existing.unify(reference)); + } } + self.alias_sets = intersection; + + println!("unify result:\n {self:?}"); self } + + /// Remember that `result` is the result of dereferencing `address`. This is important to + /// track aliasing when references are stored within other references. + fn remember_dereference(&mut self, function: &Function, address: ValueId, result: ValueId) { + if function.dfg.type_of_value(result) == Type::Reference { + if let Some(known_address) = self.get_known_value(address) { + self.expressions.insert(result, Expression::Other(known_address)); + } else { + let expression = Expression::Dereference(Box::new(Expression::Other(address))); + self.expressions.insert(result, expression.clone()); + // No known aliases to insert for this expression... can we find an alias + // even if we don't have a known address? If not we'll have to invalidate all + // known references if this reference is ever stored to. + } + } + } } impl Reference { @@ -376,8 +517,6 @@ mod tests { let func = ssa.main(); let block_id = func.entry_block(); - println!("{ssa}"); - // Store is needed by the return value, and can't be removed assert_eq!(count_stores(block_id, &func.dfg), 1); @@ -454,11 +593,14 @@ mod tests { // store Field 6 at v0 // return v3, Field 5, Field 6 // Optimized to constants 5 and 6 // } + println!("{ssa}"); let ssa = ssa.mem2reg(); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 2); + println!("{ssa}"); + // The loads should be removed assert_eq!(count_loads(main.entry_block(), &main.dfg), 0); assert_eq!(count_loads(b1, &main.dfg), 0); @@ -540,12 +682,13 @@ mod tests { // store v0 at v2 // jmp b1() // b1(): - // store Field 1 at v0 // store Field 2 at v0 // v8 = eq Field 1, Field 2 // return // } + println!("{ssa}"); let ssa = ssa.mem2reg(); + println!("{ssa}"); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 2); @@ -554,11 +697,10 @@ mod tests { assert_eq!(count_loads(main.entry_block(), &main.dfg), 0); assert_eq!(count_loads(b1, &main.dfg), 0); - println!("{ssa}"); - - // The stores are not removed in this case + // Only the first store in b1 is removed since there is another store to the same reference + // in the same block, and the store is not needed before the later store. assert_eq!(count_stores(main.entry_block(), &main.dfg), 2); - assert_eq!(count_stores(b1, &main.dfg), 2); + assert_eq!(count_stores(b1, &main.dfg), 1); let b1_instructions = main.dfg[b1].instructions(); From 88913c1ba750e2a15a5cf5c07bf6b0ba89b5a529 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 23 Aug 2023 11:52:57 -0500 Subject: [PATCH 03/16] clippy --- .../str_as_bytes/src/main.nr | 22 +++++++++---------- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr b/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr index 80a8ccbd77a..6890b156b15 100644 --- a/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr +++ b/crates/nargo_cli/tests/compile_success_empty/str_as_bytes/src/main.nr @@ -1,18 +1,18 @@ use dep::std; fn main() { let a = "hello"; - // let b = a.as_bytes(); - // assert(b[0]==104); - // assert(b[1]==101); - // assert(b[2]==108); - // assert(b[3]==108); - // assert(b[4]==111); - // assert(b.len()==5); + let b = a.as_bytes(); + assert(b[0]==104); + assert(b[1]==101); + assert(b[2]==108); + assert(b[3]==108); + assert(b[4]==111); + assert(b.len()==5); let mut c = a.as_bytes_vec(); assert(c.get(0)==104); - // assert(c.get(1)==101); - // assert(c.get(2)==108); - // assert(c.get(3)==108); - // assert(c.get(4)==111); + assert(c.get(1)==101); + assert(c.get(2)==108); + assert(c.get(3)==108); + assert(c.get(4)==111); assert(c.len()==5); } diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index e7219b05912..888bc454cda 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -373,7 +373,7 @@ impl Block { self.expressions.insert(result, Expression::Other(known_address)); } else { let expression = Expression::Dereference(Box::new(Expression::Other(address))); - self.expressions.insert(result, expression.clone()); + self.expressions.insert(result, expression); // No known aliases to insert for this expression... can we find an alias // even if we don't have a known address? If not we'll have to invalidate all // known references if this reference is ever stored to. From 8804a3f0ee79ba7b8cca60cf74738beb150854da Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 23 Aug 2023 16:00:16 -0500 Subject: [PATCH 04/16] Mild cleanup --- crates/noirc_evaluator/src/ssa.rs | 2 + crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 201 ++++++++++-------- 2 files changed, 111 insertions(+), 92 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 77399994e83..aaa18205af2 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -63,6 +63,8 @@ pub(crate) fn optimize_into_acir( // and this pass is missed, slice merging will fail inside of flattening. .mem2reg() .print(print_ssa_passes, "After Mem2Reg:") + .fold_constants() + .simplify_cfg() .flatten_cfg() .print(print_ssa_passes, "After Flattening:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 888bc454cda..2051afc341a 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -9,7 +9,7 @@ use crate::ssa::{ cfg::ControlFlowGraph, dom::DominatorTree, function::Function, - instruction::{Instruction, InstructionId}, + instruction::{Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, types::Type, value::ValueId, @@ -20,7 +20,6 @@ use crate::ssa::{ impl Ssa { /// Attempts to remove any load instructions that recover values that are already available in /// scope, and attempts to remove stores that are subsequently redundant. - /// As long as they are not stores on memory used inside of loops pub(crate) fn mem2reg(mut self) -> Ssa { for function in self.functions.values_mut() { let mut context = PerFunctionContext::new(function); @@ -47,25 +46,35 @@ struct PerFunctionContext { #[derive(Debug, Default, Clone)] 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. expressions: BTreeMap, + /// 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. aliases: BTreeMap>, - alias_sets: BTreeMap, + /// 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. + references: BTreeMap, } +/// An `Expression` here is used to represent a canonical key +/// into the aliases map since otherwise two dereferences of the +/// same address will be given different ValueIds. +/// +/// TODO: This should be expanded to any other value that can +/// hold a reference, such as arrays. #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] enum Expression { Dereference(Box), Other(ValueId), } -#[derive(Debug, Clone)] -struct Reference { - aliases: BTreeSet, - value: ReferenceValue, -} - #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum ReferenceValue { Unknown, @@ -106,10 +115,6 @@ impl PerFunctionContext { /// of the value of the same reference at the end of its predecessor blocks. fn find_starting_references(&mut self, block: BasicBlockId) -> Block { let mut predecessors = self.cfg.predecessors(block); - println!( - "find_starting_references block {block} with {} predecessor(s)", - predecessors.len() - ); if let Some(first_predecessor) = predecessors.next() { let first = self.blocks.get(&first_predecessor).cloned().unwrap_or_default(); @@ -145,8 +150,7 @@ impl PerFunctionContext { self.analyze_instruction(function, &mut references, instruction, &mut last_stores); } - let terminator_arguments = function.dfg[block].terminator_arguments(); - self.mark_all_unknown(terminator_arguments, function, &mut references, &mut last_stores); + self.handle_terminator(function, block, &mut references, &mut last_stores); // 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 @@ -194,7 +198,7 @@ impl PerFunctionContext { self.instructions_to_remove.insert(*last_store); } - references.set_known_value(address, value); + references.set_known_value(address, value, last_stores); last_stores.insert(address, instruction); } Instruction::Call { arguments, .. } => { @@ -227,7 +231,7 @@ impl PerFunctionContext { for value in values { if function.dfg.type_of_value(*value) == Type::Reference { let value = function.dfg.resolve(*value); - references.set_unknown(value); + references.set_unknown(value, last_stores); last_stores.remove(&value); } } @@ -244,6 +248,45 @@ impl PerFunctionContext { .retain(|instruction| !self.instructions_to_remove.contains(instruction)); } } + + fn handle_terminator( + &self, + function: &mut Function, + block: BasicBlockId, + references: &mut Block, + last_stores: &mut BTreeMap, + ) { + match function.dfg[block].unwrap_terminator() { + TerminatorInstruction::JmpIf { .. } => (), // Nothing to do + TerminatorInstruction::Jmp { destination, arguments, .. } => { + let destination_parameters = function.dfg[*destination].parameters(); + assert_eq!(destination_parameters.len(), arguments.len()); + + // Add an alias for each reference parameter + for (parameter, argument) in destination_parameters.iter().zip(arguments) { + if function.dfg.type_of_value(*parameter) == Type::Reference { + let argument = 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); + + // TODO: Should we also insert an expression/alias for the reverse, + // argument -> parameter? + } + } + } + } + } + TerminatorInstruction::Return { return_values } => { + // Removing all `last_stores` for each returned reference is more important here + // than setting them all to ReferenceValue::Unknown since no other block should + // have a block with a Return terminator as a predecessor anyway. + self.mark_all_unknown(return_values, function, references, last_stores); + } + } + } } impl Block { @@ -256,81 +299,69 @@ impl Block { if aliases.len() == 1 { let alias = aliases.first().expect("There should be exactly 1 alias"); - if let Some(reference) = self.alias_sets.get(alias) { - if let ReferenceValue::Known(value) = reference.value { - println!("get_known_value: returning {value} for alias {alias}"); - return Some(value); - } else { - println!("get_known_value: ReferenceValue::Unknown for alias {alias}"); - } - } else { - println!("get_known_value: No alias_set for alias {alias}"); + if let Some(ReferenceValue::Known(value)) = self.references.get(alias) { + return Some(*value); } - } else { - println!("get_known_value: {} aliases for address {address}", aliases.len()); } - } else { - println!("get_known_value: No known aliases for address {address}"); } - } else { - println!("get_known_value: No expression for address {address}"); } None } /// If the given address is known, set its value to `ReferenceValue::Known(value)`. - fn set_known_value(&mut self, address: ValueId, value: ValueId) { - self.set_value(address, ReferenceValue::Known(value)); + fn set_known_value( + &mut self, + address: ValueId, + value: ValueId, + last_stores: &mut BTreeMap, + ) { + self.set_value(address, ReferenceValue::Known(value), last_stores); } - fn set_unknown(&mut self, address: ValueId) { - self.set_value(address, ReferenceValue::Unknown); + fn set_unknown( + &mut self, + address: ValueId, + last_stores: &mut BTreeMap, + ) { + self.set_value(address, ReferenceValue::Unknown, last_stores); } - fn set_value(&mut self, address: ValueId, value: ReferenceValue) { - // TODO: Verify this does not fail in the case of reference parameters - if let Some(expression) = self.expressions.get(&address) { - if let Some(aliases) = self.aliases.get(expression) { - if aliases.is_empty() { - // uh-oh, we don't know at all what this reference refers to, could be anything. - // Now we have to invalidate every reference we know of - todo!("empty alias set"); - } else if aliases.len() == 1 { - let alias = aliases.first().expect("There should be exactly 1 alias"); - - if let Some(reference) = self.alias_sets.get_mut(alias) { - println!("set_known_value: Set value to {value:?} for alias {alias}"); - reference.value = value; - } else { - let reference = Reference { value, aliases: BTreeSet::new() }; - self.alias_sets.insert(*alias, reference); - println!("set_known_value: Created new alias set for alias {alias}, with value {value:?}"); - } - } else { - println!("set_known_value: {} aliases for expression {expression:?}, marking all unknown", aliases.len()); - // More than one alias. We're not sure which it refers to so we have to - // conservatively invalidate all references it may refer to. - for alias in aliases.iter() { - if let Some(reference) = self.alias_sets.get_mut(alias) { - println!(" Marking {alias} unknown"); - reference.value = ReferenceValue::Unknown; - } else { - println!(" {alias} already unknown"); - } - } + fn set_value( + &mut self, + address: ValueId, + value: ReferenceValue, + last_stores: &mut BTreeMap, + ) { + let expression = self.expressions.entry(address).or_insert(Expression::Other(address)); + let aliases = self.aliases.entry(expression.clone()).or_default(); + + if aliases.is_empty() { + // uh-oh, we don't know at all what this reference refers to, could be anything. + // Now we have to invalidate every reference we know of. + self.invalidate_all_references(last_stores); + } else if aliases.len() == 1 { + let alias = aliases.first().expect("There should be exactly 1 alias"); + self.references.insert(*alias, value); + } 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. + for alias in aliases.iter() { + if let Some(reference_value) = self.references.get_mut(alias) { + *reference_value = ReferenceValue::Unknown; } - } else { - println!("set_known_value: ReferenceValue::Unknown for expression {expression:?}"); } - } else { - println!("\n\n!! set_known_value: Creating fresh expression for {address} in set_value... aliases will be empty\n\n"); - self.expressions.insert(address, Expression::Other(address)); } } - fn unify(mut self, other: &Self) -> Self { - println!("unify:\n {self:?}\n {other:?}"); + fn invalidate_all_references(&mut self, last_stores: &mut BTreeMap) { + for reference_value in self.references.values_mut() { + *reference_value = ReferenceValue::Unknown; + } + + last_stores.clear(); + } + fn unify(mut self, other: &Self) -> Self { for (value_id, expression) in &other.expressions { if let Some(existing) = self.expressions.get(value_id) { assert_eq!(existing, expression, "Expected expressions for {value_id} to be equal"); @@ -354,14 +385,13 @@ impl Block { // Keep only the references present in both maps. let mut intersection = BTreeMap::new(); - for (value_id, reference) in &other.alias_sets { - if let Some(existing) = self.alias_sets.get(value_id) { - intersection.insert(*value_id, existing.unify(reference)); + for (value_id, reference) in &other.references { + if let Some(existing) = self.references.get(value_id) { + intersection.insert(*value_id, existing.unify(*reference)); } } - self.alias_sets = intersection; + self.references = intersection; - println!("unify result:\n {self:?}"); self } @@ -382,14 +412,6 @@ impl Block { } } -impl Reference { - fn unify(&self, other: &Self) -> Self { - let value = self.value.unify(other.value); - let aliases = self.aliases.union(&other.aliases).copied().collect(); - Self { value, aliases } - } -} - impl ReferenceValue { fn unify(self, other: Self) -> Self { if self == other { @@ -593,14 +615,11 @@ mod tests { // store Field 6 at v0 // return v3, Field 5, Field 6 // Optimized to constants 5 and 6 // } - println!("{ssa}"); let ssa = ssa.mem2reg(); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 2); - println!("{ssa}"); - // The loads should be removed assert_eq!(count_loads(main.entry_block(), &main.dfg), 0); assert_eq!(count_loads(b1, &main.dfg), 0); @@ -686,9 +705,7 @@ mod tests { // v8 = eq Field 1, Field 2 // return // } - println!("{ssa}"); let ssa = ssa.mem2reg(); - println!("{ssa}"); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 2); From cd29ec842361cda7fba0ab2534e84152a5610742 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 23 Aug 2023 16:11:13 -0500 Subject: [PATCH 05/16] Undo some unneeded changes --- crates/noirc_evaluator/src/ssa.rs | 2 -- crates/noirc_evaluator/src/ssa/ir/basic_block.rs | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index aaa18205af2..77399994e83 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -63,8 +63,6 @@ pub(crate) fn optimize_into_acir( // and this pass is missed, slice merging will fail inside of flattening. .mem2reg() .print(print_ssa_passes, "After Mem2Reg:") - .fold_constants() - .simplify_cfg() .flatten_cfg() .print(print_ssa_passes, "After Flattening:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores diff --git a/crates/noirc_evaluator/src/ssa/ir/basic_block.rs b/crates/noirc_evaluator/src/ssa/ir/basic_block.rs index b3b566f8f37..998591f7210 100644 --- a/crates/noirc_evaluator/src/ssa/ir/basic_block.rs +++ b/crates/noirc_evaluator/src/ssa/ir/basic_block.rs @@ -122,11 +122,10 @@ impl BasicBlock { /// Return the jmp arguments, if any, of this block's TerminatorInstruction. /// - /// If this block has no terminator, this will be empty. + /// If this block has no terminator, or a Return terminator this will be empty. pub(crate) fn terminator_arguments(&self) -> &[ValueId] { match &self.terminator { Some(TerminatorInstruction::Jmp { arguments, .. }) => arguments, - Some(TerminatorInstruction::Return { return_values, .. }) => return_values, _ => &[], } } From f3f34aa4f295c9233f6c75a0c751de688abb44d9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 24 Aug 2023 12:10:36 -0500 Subject: [PATCH 06/16] Partially revert cleanup commit which broke some tests --- .../execution_success/references/src/main.nr | 2 +- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 37 +++++++++++-------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/crates/nargo_cli/tests/execution_success/references/src/main.nr b/crates/nargo_cli/tests/execution_success/references/src/main.nr index ec23f2f3a38..70de5cada3f 100644 --- a/crates/nargo_cli/tests/execution_success/references/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/references/src/main.nr @@ -229,4 +229,4 @@ fn regression_2218_loop(x: Field, y: Field) { assert(*q1 == 20); } assert(*q1 == 2); -} \ No newline at end of file +} diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 2051afc341a..c0141fe0f8d 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -332,22 +332,27 @@ impl Block { value: ReferenceValue, last_stores: &mut BTreeMap, ) { - let expression = self.expressions.entry(address).or_insert(Expression::Other(address)); - let aliases = self.aliases.entry(expression.clone()).or_default(); - - if aliases.is_empty() { - // uh-oh, we don't know at all what this reference refers to, could be anything. - // Now we have to invalidate every reference we know of. - self.invalidate_all_references(last_stores); - } else if aliases.len() == 1 { - let alias = aliases.first().expect("There should be exactly 1 alias"); - self.references.insert(*alias, value); - } 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. - for alias in aliases.iter() { - if let Some(reference_value) = self.references.get_mut(alias) { - *reference_value = ReferenceValue::Unknown; + if let Some(expression) = self.expressions.get(&address) { + if let Some(aliases) = self.aliases.get(expression) { + if aliases.is_empty() { + // uh-oh, we don't know at all what this reference refers to, could be anything. + // Now we have to invalidate every reference we know of + println!("Invalidating all references for address {address}"); + self.invalidate_all_references(last_stores); + } else if aliases.len() == 1 { + let alias = aliases.first().expect("There should be exactly 1 alias"); + println!("set_known_value: Setting {} value to {:?}", alias, value); + self.references.insert(*alias, value); + } else { + println!("set_known_value: {} aliases for expression {expression:?}, marking all unknown", aliases.len()); + // More than one alias. We're not sure which it refers to so we have to + // conservatively invalidate all references it may refer to. + for alias in aliases.iter() { + println!(" Marking {alias} unknown"); + if let Some(reference_value) = self.references.get_mut(alias) { + *reference_value = ReferenceValue::Unknown; + } + } } } } From 0887db5139d3ab05e648a67937e627ecba073875 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 24 Aug 2023 12:55:36 -0500 Subject: [PATCH 07/16] Add large doc comment --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 67 ++++++++++++++++++- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index c0141fe0f8d..4bc4fbc5d59 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -1,6 +1,66 @@ -//! mem2reg implements a pass for promoting values stored in memory to values in registers where -//! possible. This is particularly important for converting our memory-based representation of -//! mutable variables into values that are easier to manipulate. +//! The goal of the mem2reg SSA optimization pass is to replace any `Load` instructions to known +//! addresses with the value stored at that address, if it is also known. This pass will also remove +//! any `Store` instructions within a block that are no longer needed because no more loads occur in +//! between the Store in question and the next Store. +//! +//! The pass works as follows: +//! - Each block in each function is iterated in forward-order. +//! - The starting value of each reference in the block is the unification of the same references +//! at the end of each direct predecessor block to the current block. +//! - At each step, the value of each reference is either Known(ValueId) or Unknown. +//! - Two reference values unify to each other if they are exactly equal, or to Unknown otherwise. +//! - If a block has no predecessors, the starting value of each reference is Unknown. +//! - Throughout this pass, aliases of each reference are also tracked. +//! - References typically have 1 alias - themselves. +//! - A reference with multiple aliases means we will not be able to optimize out loads if the +//! reference is stored to. Note that this means we can still optimize out loads if these +//! aliased references are never stored to, or the store occurs after a load. +//! - A reference with 0 aliases means we were unable to find which reference this reference +//! refers to. If such a reference is stored to, we must conservatively invalidate every +//! reference in the current block. +//! +//! From there, to figure out the value of each reference at the end of block, iterate each instruction: +//! - On `Instruction::Allocate`: +//! - Register a new reference was made with itself as its only alias +//! - On `Instruction::Load { address }`: +//! - If `address` is known to only have a single alias (including itself) and if the value of +//! that alias is known, replace the value of the load with the known value. +//! - Furthermore, if the result of the load is a reference, mark that reference as an alias +//! of the reference it dereferences to (if known). +//! - If which reference it dereferences to is not known, this load result has no aliases. +//! - On `Instruction::Store { address, value }`: +//! - If the address of the store is known: +//! - If the address has exactly 1 alias: +//! - Set the value of the address to `Known(value)`. +//! - If the address has more than 1 alias: +//! - Set the value of every possible alias to `Unknown`. +//! - If the address has 0 aliases: +//! - Conservatively mark every alias in the block to `Unknown`. +//! - If the address of the store is not known: +//! - Conservatively mark every alias in the block to `Unknown`. +//! - Additionally, if there were no Loads to any alias of the address between this Store and +//! the previous Store to the same address, the previous store can be removed. +//! - On `Instruction::Call { arguments }`: +//! - If any argument of the call is a reference, set the value of each alias of that +//! reference to `Unknown` +//! - Any builtin functions that may return aliases if their input also contains a +//! reference should be tracked. Examples: `slice_push_back`, `slice_insert`, `slice_remove`, etc. +//! +//! On a terminator instruction: +//! - If the terminator is a `Jmp`: +//! - For each reference argument of the jmp, mark the corresponding block parameter it is passed +//! to as an alias for the jmp argument. +//! +//! Finally, if this is the only block in the function, we can remove any Stores that were not +//! referenced by the terminator instruction. +//! +//! Repeating this algorithm for each block in the function in program order should result in +//! optimizing out most known loads. However, identifying all aliases correctly has been proven +//! undecidable in general (Landi, 1992). So this pass will not always optimize out all loads +//! that could theoretically be optimized out. This pass can be performed at any time in the +//! SSA optimization pipeline, although it will be more successful the simpler the program's CFG is. +//! This pass is currently performed several times to enable other passes - most notably being +//! performed before loop unrolling to try to allow for mutable variables used for for loop indices. use std::collections::{BTreeMap, BTreeSet}; use crate::ssa::{ @@ -75,6 +135,7 @@ enum Expression { Other(ValueId), } +/// Every reference's value is either Known and can be optimized away, or Unknown. #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum ReferenceValue { Unknown, From 33feb07991629c9ef127ecf7d516b6164f42255c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 24 Aug 2023 13:14:15 -0500 Subject: [PATCH 08/16] Add test and remove printlns --- .../references_aliasing/Nargo.toml | 7 +++++++ .../references_aliasing/Prover.toml | 0 .../references_aliasing/src/main.nr | 10 ++++++++++ .../target/references.bytecode | 1 + .../references_aliasing/target/witness.tr | Bin 0 -> 788 bytes crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 4 ---- 6 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/Prover.toml create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode create mode 100644 crates/nargo_cli/tests/execution_success/references_aliasing/target/witness.tr diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml b/crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml new file mode 100644 index 00000000000..b52fdcf77f0 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "references" +type = "bin" +authors = [""] +compiler_version = "0.5.1" + +[dependencies] diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/Prover.toml b/crates/nargo_cli/tests/execution_success/references_aliasing/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr b/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr new file mode 100644 index 00000000000..4582444c8f7 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/src/main.nr @@ -0,0 +1,10 @@ +fn increment(mut r: &mut Field) { + *r = *r + 1; +} + +fn main() { + let mut x = 100; + let mut xref = &mut x; + increment(xref); + assert(*xref == 101); +} diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode b/crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode new file mode 100644 index 00000000000..3e521923327 --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/references_aliasing/target/references.bytecode @@ -0,0 +1 @@ +H4sIAAAAAAAA/+1c8ZONVRh+WCKykYqIiFRE37f37u5dkY2IEEJEtLv2bkJKQkpCSkJKIjLGNE3TmJqpmZqpmZrRH9L/Uu907szZzfjlPO+Z98yc88u+w8yz3/O853ue93zf3fsXgL/x/zXY/ex0P4uwVQ7ysCpFW7Vab2+pl5Wyu2jp6Km1FtXWnrZaWStba629LbVKpV6r1to7ejrai46yWqmXfa0dlT4HNpiAVe/7bzX9izHoJvwHkfkP5mEV/vU2efWQAb3z//82BU4Y8HsG6th8k3+j/nKNJjUp4A4Bb/Nr8R7C71HhQZrXtLEsG99QpGd8Q6FjfLd5dTa+QMyhSkINg23jE97D+D1SNT62po1l2fiGIz3jGw4d47vdq7PxBWIOd4KycUfAtvEJ7xH8HqkaH1vTxrJsfCORnvGNhI7x3eHV2fgCMUc6Qdm4o2Db+IT3KH6PVI2PrWljWTa+ZtCMrxbL+JqhY3x3enU2vkDMZicoG3c0bBuf8B7N71EyhjIGLEOp12MZyhjoGMpdXp0NJRBzjBOUjTsWtg1FeI/l9+iWhhKqA9Ok74bOTcHmzNxH9yTCmdnneyNxLsJWL7PP43jX1SIYbO+Rnoy7CW4o76vQ8bEmKv+yytzf44l9uQrWkBXvcRWRf78h6z6vzkNWIOZ4JygbdwJsD1nCewK/R6qPq9iaNhZ7SGCeLifS9CsrsYyPd839je9+r87GF4g50QnKxp0E28YnvCfxe6T5uKqVqelkIudYhsK8Zv96H/DqbCiBmJOdoGzcKbBtKMJ7Cr9HqpPUFMSZpIqwRX1OP5WAFfsIORU6xvegV2fjC8Sc6gRl406DbeMT3tP4PUrGUKaDZSjxXvxNh46hPOTV2VACMac7Qdm4M2DbUIT3DH6PVJ/1ME36YejcFGzOzH30SCKcmX1+NBLnImz1Mvs8k3ddKi/+pCczwX/xdw06PsZ+8cfc37OIfbkG2pDVG2vIIvLvN2Q95tV5yArEnOUEZePOhu0hS3jP5vdI9XEVW9PGsny6nAOW8cX7nPoc6Bjf416djS8Qc44TlI1bwLbxCe+C3yNFQylbmJqWRM6xDIV5zf71tnh1NpRAzNIJysatwLahCO8Kv0eqkxRb08ayPElVkd4kVYWO8bV6dTa+QMyqE5SN2wbbxie82/g9SsZQ2pGeobRDx1BqXp0NJRCz3QnKxu2AbUMR3h38HiVjKHORnqHMhY6hPOHV2VACMec6Qdm482DbUIT3PH6PkjGU+UjPUOZDx1Ce9OpsKIGY852gbNwFsG0ownsBv0fJGEon0jOUTugYylNenQ0lELPTCcrGXQjbhiI4C/k9SsZQFiE9Q1kEHUN52quzoQRiLnKCsnEXw7ahCO/F/B4lYyhLwDKUeH+NsQQ6hvKMV2dDCcRc4gRl4y6FbUMR3kv5PUrGUJYhPUNZBh1Dedars6EEYi5zgrJxl8O2oQjv5fweqf55F9OkV/A4t9yKcxG2qCa6EumZ6EromOhzXp1NNBBzpROUjbsKtk1UeK/i90jlWsXsV4D/N3XfQCc8mkjX2fiWEGYgryb2hahfMkG0BukF0RroBNHzXp2DKBBzjROUjbsWtoNIeK/l90jlWiUwV4MfRN8ijSBiDjXriH0h6pdMEK1HekG0HjpB9IJX5yAKxFzvBGXjboDtIBLeG/g9UrlWCcx14AfRd0gjiJhDzUZiX4j63TKIQjkzX0K/CNt+Jvf0RoV75Xukca8wfXcTsS9E/VS+yUn29SaFfXM90r4pglZZZXrEZmJfrhPvjViDL5F/v8H3Ja/Og28g5mYnKBt3C2wPvsJ7C79Hqt8/wNa0sSyf+LeCZnzRPuq6FTrG97JXZ+MLxNzqBGXjdsG28QnvLn6PkjGUbqRnKN3QMZQer86GEojZ7QRl426DbUMR3tv4PVL9ZFoXEasXOjcFmzNzH9UjcS7CFjU4+sAKjnjvnvqgExyveHUOjkDMPicoG3c7bAeH8N7O75FqcDBN9FWkERzMfbSDx7nfuyLLz4F3Evus9RzdcvDuAit4453YdkEneF/z6hy8gZi7nKBs3N2wHbzCeze/R8mc2F5HGsHL3EdvROJchC1qcOwBKzjindj2QCc43vTqHByBmHucoGzcvbAdHMJ7L79HqsHBNNG3kEZwMPfRPh5nlU8syYl8J/ifWPoB3P3N5i1PD3Yo8P4ROvd1E/k69xO1JPa61NLP8qB1AKxBK94J/QB0Bq23vToPWoGYB5ygbNyDsD1oCe+D/B4lc0J/Bzo3BZszcx+9G4lzEbaowXEIrOCId0I/BJ3geM+rc3AEYh5ygrJxD8N2cAjvw/weqQYH00TfRxrBwdxHR3icVU7o8gRmP/gn1Z/A3d9s3vK0aJ8C75+hc1+zT+hHiVoSe11q6Wd50DoG1qAV74R+DDqD1gdenQetQMxjTlA27nHYHrSE93F+j5I5oX8InZuCzZm5jz6KxLkIW9TgOAFWcMQ7oZ+ATnB87NU5OAIxTzhB2bgnYTs4hPdJfo9Ug4Npop8gjeBg7qNTPM4qJ3R5AnMU/JPqL+DubzZveVp0RIH3r9C5r9kn9NNELYm9LrX0szxonQFr0Ip3Qj8DnUHrU6/Og1Yg5hknKBv3LGwPWsL7LL9HyZzQP4POTcHmzNxHn0fiXIQtanCcAys44p3Qz0EnOL7w6hwcgZjnnKBs3POwHRzC+zy/R6rBwTTRL5FGcDD30QUeZ5UTujyBOQ3+SfU3cPc3m7c8LTqlwPt36NzX7BP6RaKWxF6XWvpZHqi/gu28lgy4qHCv/AHbHiF5dUGB959IwyMuEbUk9rpk68feN3I/X1LYNzcQZ98UQausdhE5Xyb25QbhumJ/1zWRf78D7ddenQ+0gZiXnaBs3CuwfaAV3lf4PVL9rmumpoMGXKO//gGokXOuaucAAA== diff --git a/crates/nargo_cli/tests/execution_success/references_aliasing/target/witness.tr b/crates/nargo_cli/tests/execution_success/references_aliasing/target/witness.tr new file mode 100644 index 0000000000000000000000000000000000000000..22a1c7fe1098dfe81652877dc26d99dc3aae67ec GIT binary patch literal 788 zcmV+v1MB=BiwFP!00002|E-o)&l5oyg>iRxciq{U9YH_b-JPB84DRmk?ixY}A%p;d z1QJLf;iu7laMAnk*|te9nkVO+_dV13T<_mgzx=8B|2?VOlQ!U3Nkfp;h+~Pypspq$ zt10NQ8K|o{$Z7$4YzgXW1+rR$9@|v0WTKS{CJjQ0JV~9mS)wwpt*|1qS&>>PgH_fz z?@Z!^l0n4G$q;LwyloXLbtbnZof;E_R$eFGr&fjXubniBiiOLBNGbp+qz}2w)2LO+ z#8kQRTebu9YR`F0I#jVlR55C+o$%VHp(unavRv7a>$H@3KXx_(IY`!qd?YZF#9p!{c9|E|4P8T1jtH3mIUXhz&U2%95tAi z0Xj0p_(5%xe{x*J?1Y zHQ+sZEvRc9nAducwE@g)BX|ec1l|EQgL&nkM-O@|fgTHRCj!Wd;90l@JPWsi*>3~$ z+76yyJ3!V>utU4R4($f#xCfl$UXZm9?9hI&LkGa@4}y6e0{8wf$T|Xc=qT8sW8fT* zgL6CqvQB~>It6y}^6cIW}vp@(4hkHEYhgM0r3WIY8vJ_B_<2j}<# zWW5CQdIje78ua)EWW5Dh??Bdjko5ta<3~`}Cs5ZHP}f&b*EdktcTm?4P}fgT*Dp}l SZ&24Cko6Z+MM~zdGXMaV18<%H literal 0 HcmV?d00001 diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 4bc4fbc5d59..19e752d8b6a 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -398,18 +398,14 @@ impl Block { if aliases.is_empty() { // uh-oh, we don't know at all what this reference refers to, could be anything. // Now we have to invalidate every reference we know of - println!("Invalidating all references for address {address}"); self.invalidate_all_references(last_stores); } else if aliases.len() == 1 { let alias = aliases.first().expect("There should be exactly 1 alias"); - println!("set_known_value: Setting {} value to {:?}", alias, value); self.references.insert(*alias, value); } else { - println!("set_known_value: {} aliases for expression {expression:?}, marking all unknown", aliases.len()); // More than one alias. We're not sure which it refers to so we have to // conservatively invalidate all references it may refer to. for alias in aliases.iter() { - println!(" Marking {alias} unknown"); if let Some(reference_value) = self.references.get_mut(alias) { *reference_value = ReferenceValue::Unknown; } From 2574c33f9b8dcc8e96840eba5b21a48c4d1c6ee3 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 24 Aug 2023 14:33:24 -0500 Subject: [PATCH 09/16] Remove todos --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 19e752d8b6a..bd1015245bf 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -126,9 +126,6 @@ struct Block { /// An `Expression` here is used to represent a canonical key /// into the aliases map since otherwise two dereferences of the /// same address will be given different ValueIds. -/// -/// TODO: This should be expanded to any other value that can -/// hold a reference, such as arrays. #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)] enum Expression { Dereference(Box), @@ -203,7 +200,6 @@ impl PerFunctionContext { block: BasicBlockId, mut references: Block, ) { - // TODO: Can we avoid cloning here? let instructions = function.dfg[block].instructions().to_vec(); let mut last_stores = BTreeMap::new(); @@ -274,10 +270,6 @@ impl PerFunctionContext { aliases.insert(result); references.aliases.insert(Expression::Other(result), aliases); } - - // TODO: Track aliases here - // Instruction::ArrayGet { array, index } => todo!(), - // Instruction::ArraySet { array, index, value } => todo!(), _ => (), } } @@ -332,9 +324,6 @@ impl PerFunctionContext { if let Some(aliases) = references.aliases.get_mut(expression) { // The argument reference is possibly aliased by this block parameter aliases.insert(*parameter); - - // TODO: Should we also insert an expression/alias for the reverse, - // argument -> parameter? } } } From ad501982b073d83e28e6de9327f451516e0c6888 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 12:09:41 -0500 Subject: [PATCH 10/16] Do not remove stores which may alias function parameters --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 38 ++++++++++++++++--- .../src/monomorphization/mod.rs | 4 +- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index bd1015245bf..65c7f15e311 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -55,12 +55,12 @@ //! referenced by the terminator instruction. //! //! Repeating this algorithm for each block in the function in program order should result in -//! optimizing out most known loads. However, identifying all aliases correctly has been proven +//! optimizing out most known loads. However, identifying all aliases correctly has been proven //! undecidable in general (Landi, 1992). So this pass will not always optimize out all loads //! that could theoretically be optimized out. This pass can be performed at any time in the //! SSA optimization pipeline, although it will be more successful the simpler the program's CFG is. //! This pass is currently performed several times to enable other passes - most notably being -//! performed before loop unrolling to try to allow for mutable variables used for for loop indices. +//! performed before loop unrolling to try to allow for mutable variables used for for loop indices. use std::collections::{BTreeMap, BTreeSet}; use crate::ssa::{ @@ -213,14 +213,40 @@ impl PerFunctionContext { // 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 { - for (_, instruction) in last_stores { - self.instructions_to_remove.insert(instruction); - } + self.remove_stores_that_do_not_alias_parameters(function, &references, last_stores); } self.blocks.insert(block, references); } + /// 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, + function: &Function, + references: &Block, + last_stores: BTreeMap, + ) { + let reference_parameters = function + .parameters() + .iter() + .filter(|param| function.dfg.type_of_value(**param) == Type::Reference) + .collect::>(); + + for (allocation, instruction) in last_stores { + if let Some(expression) = references.expressions.get(&allocation) { + if let Some(aliases) = references.aliases.get(expression) { + let allocation_aliases_parameter = + !aliases.iter().any(|alias| reference_parameters.contains(alias)); + + if !aliases.is_empty() && !allocation_aliases_parameter { + self.instructions_to_remove.insert(instruction); + } + } + } + } + } + fn analyze_instruction( &mut self, function: &mut Function, @@ -481,6 +507,7 @@ mod tests { use im::vector; use crate::ssa::{ + function_builder::FunctionBuilder, ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, @@ -489,7 +516,6 @@ mod tests { map::Id, types::Type, }, - ssa_builder::FunctionBuilder, }; #[test] diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 2ef980176d3..fb441a1bc17 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -882,8 +882,8 @@ impl<'interner> Monomorphizer<'interner> { } } let printable_type: PrintableType = typ.into(); - let abi_as_string = - serde_json::to_string(&printable_type).expect("ICE: expected PrintableType to serialize"); + let abi_as_string = serde_json::to_string(&printable_type) + .expect("ICE: expected PrintableType to serialize"); arguments.push(ast::Expression::Literal(ast::Literal::Str(abi_as_string))); } From 71e4955d93870dd4748ace73468dc544a282e3de Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 12:20:58 -0500 Subject: [PATCH 11/16] Fix extra 'not' --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 65c7f15e311..2bbee618d63 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -237,12 +237,14 @@ impl PerFunctionContext { if let Some(expression) = references.expressions.get(&allocation) { if let Some(aliases) = references.aliases.get(expression) { let allocation_aliases_parameter = - !aliases.iter().any(|alias| reference_parameters.contains(alias)); + aliases.iter().any(|alias| reference_parameters.contains(alias)); if !aliases.is_empty() && !allocation_aliases_parameter { self.instructions_to_remove.insert(instruction); } } + } else { + self.instructions_to_remove.insert(instruction); } } } @@ -507,7 +509,7 @@ mod tests { use im::vector; use crate::ssa::{ - function_builder::FunctionBuilder, + ssa_builder::FunctionBuilder, ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, From 6ca640f2c8b80691d97b3b3dce2b1c65ef8cf958 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 25 Aug 2023 12:30:27 -0500 Subject: [PATCH 12/16] Update crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Maxim Vezenov --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 2bbee618d63..cedb26a43aa 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -25,7 +25,7 @@ //! - On `Instruction::Load { address }`: //! - If `address` is known to only have a single alias (including itself) and if the value of //! that alias is known, replace the value of the load with the known value. -//! - Furthermore, if the result of the load is a reference, mark that reference as an alias +//! - Furthermore, if the result of the load is a reference, mark the result as an alias //! of the reference it dereferences to (if known). //! - If which reference it dereferences to is not known, this load result has no aliases. //! - On `Instruction::Store { address, value }`: From 34d8aea16bd5a6e4780081acf096f19eb9150475 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 25 Aug 2023 12:32:43 -0500 Subject: [PATCH 13/16] Update crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Maxim Vezenov --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index cedb26a43aa..b8a031f1aa4 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -60,7 +60,7 @@ //! that could theoretically be optimized out. This pass can be performed at any time in the //! SSA optimization pipeline, although it will be more successful the simpler the program's CFG is. //! This pass is currently performed several times to enable other passes - most notably being -//! performed before loop unrolling to try to allow for mutable variables used for for loop indices. +//! performed before loop unrolling to try to allow for mutable variables used for loop indices. use std::collections::{BTreeMap, BTreeSet}; use crate::ssa::{ From c59e2d3246b1b256e5a520d7a3cc2015cf6447e2 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 25 Aug 2023 12:45:23 -0500 Subject: [PATCH 14/16] Update crates/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Maxim Vezenov --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index b8a031f1aa4..4eb58de3ead 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -29,7 +29,7 @@ //! of the reference it dereferences to (if known). //! - If which reference it dereferences to is not known, this load result has no aliases. //! - On `Instruction::Store { address, value }`: -//! - If the address of the store is known: +//! - If the address of the store is `Known(value)`: //! - If the address has exactly 1 alias: //! - Set the value of the address to `Known(value)`. //! - If the address has more than 1 alias: From 643cfc77f6d474b5cf2e993f4687407779c2ccad Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 12:45:37 -0500 Subject: [PATCH 15/16] Fix confusing wording --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 2bbee618d63..fc5be2a0794 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -60,7 +60,7 @@ //! that could theoretically be optimized out. This pass can be performed at any time in the //! SSA optimization pipeline, although it will be more successful the simpler the program's CFG is. //! This pass is currently performed several times to enable other passes - most notably being -//! performed before loop unrolling to try to allow for mutable variables used for for loop indices. +//! performed before loop unrolling to try to allow for mutable variables used for loop indices. use std::collections::{BTreeMap, BTreeSet}; use crate::ssa::{ From bf2f9f3551d083ece20ed736bb2d1ee7df14d786 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 25 Aug 2023 12:45:56 -0500 Subject: [PATCH 16/16] formatting --- crates/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs index 4eb58de3ead..3e63aec63b4 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -509,7 +509,6 @@ mod tests { use im::vector; use crate::ssa::{ - ssa_builder::FunctionBuilder, ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, @@ -518,6 +517,7 @@ mod tests { map::Id, types::Type, }, + ssa_builder::FunctionBuilder, }; #[test]