From bd3e685c448981474e24da130e1245529c32df9b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 18:50:26 +0000 Subject: [PATCH 01/58] remove any unused loads and if we have a single store in a func, delete it --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 206 +++++++++++++++++- 1 file changed, 199 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 3d98f4126cf..b63c3f3c7b0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -116,7 +116,45 @@ struct PerFunctionContext<'f> { /// 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, + last_loads: HashMap, + + /// Track whether a load result was used across all blocks. + load_results: HashMap, + + /// Track whether a reference was passed into another entry point + stores_used_in_calls: HashMap>, + + /// Flag for tracking whether we had to perform a re-load as part of the Brillig CoW optimization. + /// Stores made as part of this optimization should not be removed. + /// We want to catch stores of this nature: + /// ```text + /// v3 = load v1 + // inc_rc v3 + // v4 = load v1 + // inc_rc v4 + // store v4 at v1 + // store v3 at v2 + /// ``` + /// + /// We keep track of an optional boolean flag as we go through instructions. + /// If the flag exists it means we have hit a load instruction. + /// If the flag is false it means we have processed a single load, while if the flag is true + /// it means we have performed a re-load. + /// The field is reset to `None` on every instruction that is not a load, inc_rc, dec_rc, or function call. + inside_rc_reload: Option, +} + +#[derive(Debug, Clone)] +struct PerFuncLoadResultContext { + load_counter: u32, + load_instruction: InstructionId, + instructions_using_result: Vec<(InstructionId, BasicBlockId)>, +} + +impl PerFuncLoadResultContext { + fn new(load_instruction: InstructionId) -> Self { + Self { load_counter: 0, load_instruction, instructions_using_result: vec![] } + } } impl<'f> PerFunctionContext<'f> { @@ -131,6 +169,9 @@ impl<'f> PerFunctionContext<'f> { blocks: BTreeMap::new(), instructions_to_remove: BTreeSet::new(), last_loads: HashMap::default(), + load_results: HashMap::default(), + inside_rc_reload: None, + stores_used_in_calls: HashMap::default(), } } @@ -148,6 +189,27 @@ impl<'f> PerFunctionContext<'f> { self.analyze_block(block, references); } + let mut loads_removed = HashMap::default(); + for (_, PerFuncLoadResultContext { load_counter, load_instruction, .. }) in + self.load_results.iter() + { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + panic!("Should only have a load instruction here"); + }; + + if *load_counter == 0 { + if let Some(counter) = loads_removed.get_mut(&address) { + *counter += 1; + } else { + loads_removed.insert(address, 1); + } + + self.instructions_to_remove.insert(*load_instruction); + } + } + + let mut not_removed_stores: HashMap = HashMap::default(); // 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_id, block) in self.blocks.iter() { @@ -169,15 +231,103 @@ impl<'f> PerFunctionContext<'f> { let last_load_not_in_return = self .last_loads .get(store_address) - .map(|(_, last_load_block)| *last_load_block != *block_id) + .map(|(_, last_load_block, _)| *last_load_block != *block_id) .unwrap_or(true); !is_return_value && last_load_not_in_return + } else if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = + (self.last_loads.get(store_address), loads_removed.get(store_address)) + { + *last_loads_counter == *loads_removed_counter } else { self.last_loads.get(store_address).is_none() }; - if remove_load && !is_reference_param { + let is_not_used_in_reference_param = + self.stores_used_in_calls.get(store_address).is_none(); + if remove_load && !is_reference_param && is_not_used_in_reference_param { self.instructions_to_remove.insert(*store_instruction); + if let Some((_, counter)) = not_removed_stores.get_mut(store_address) { + *counter -= 1; + } + } else if let Some((_, counter)) = not_removed_stores.get_mut(store_address) { + *counter += 1; + } else { + not_removed_stores.insert(*store_address, (*store_instruction, 1)); + } + } + } + + self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, .. }| { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + panic!("Should only have a load instruction here"); + }; + not_removed_stores.contains_key(&address) + }); + + let mut new_instructions = HashMap::default(); + for (store_address, (store_instruction, store_counter)) in not_removed_stores { + let Instruction::Store { value, .. } = self.inserter.function.dfg[store_instruction] + else { + panic!("Should only have a store instruction"); + }; + + if store_counter != 0 { + continue; + } + self.instructions_to_remove.insert(store_instruction); + + if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = + (self.last_loads.get(&store_address), loads_removed.get(&store_address)) + { + if *last_loads_counter < *loads_removed_counter { + panic!("The number of loads removed should not be more than all loads"); + } + } + + for ( + result, + PerFuncLoadResultContext { + load_counter, + load_instruction, + instructions_using_result, + }, + ) in self.load_results.iter() + { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + panic!("Should only have a load instruction here"); + }; + if address != store_address { + continue; + } + + if *load_counter > 0 { + self.inserter.map_value(*result, value); + for (instruction, block_id) in instructions_using_result { + let new_instruction = + self.inserter.push_instruction(*instruction, *block_id); + if let Some(new_instruction) = new_instruction { + new_instructions + .insert((*instruction, block_id), Some(new_instruction)); + } else { + new_instructions.insert((*instruction, block_id), None); + } + } + + self.instructions_to_remove.insert(*load_instruction); + } + } + } + + // Re-assign or delete any mapped instructions after the final loads were removed. + for ((old_instruction, block_id), new_instruction) in new_instructions { + let instructions = self.inserter.function.dfg[*block_id].instructions_mut(); + if let Some(index) = instructions.iter().position(|v| *v == old_instruction) { + if let Some(new_instruction) = new_instruction { + instructions[index] = new_instruction; + } else { + instructions.remove(index); } } } @@ -266,6 +416,16 @@ impl<'f> PerFunctionContext<'f> { return; } + self.inserter.function.dfg[instruction].for_each_value(|value| { + if let Some(PerFuncLoadResultContext { + load_counter, instructions_using_result, .. + }) = self.load_results.get_mut(&value) + { + *load_counter += 1; + instructions_using_result.push((instruction, block_id)); + } + }); + match &self.inserter.function.dfg[instruction] { Instruction::Load { address } => { let address = self.inserter.function.dfg.resolve(*address); @@ -275,13 +435,20 @@ impl<'f> PerFunctionContext<'f> { // 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 = self.inserter.function.dfg.instruction_results(instruction)[0]; 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)); + self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); + + let load_counter = + if let Some((_, _, load_counter)) = self.last_loads.get(&address) { + *load_counter + 1 + } else { + 1 + }; + self.last_loads.insert(address, (instruction, block_id, load_counter)); } } Instruction::Store { address, value } => { @@ -294,6 +461,11 @@ impl<'f> PerFunctionContext<'f> { // function calls in-between, we can remove the previous store. if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); + if let Some(PerFuncLoadResultContext { load_counter, .. }) = + self.load_results.get_mut(&value) + { + *load_counter -= 1; + } } references.set_known_value(address, value); @@ -347,7 +519,17 @@ impl<'f> PerFunctionContext<'f> { references.aliases.insert(expression, aliases); } } - Instruction::Call { arguments, .. } => self.mark_all_unknown(arguments, references), + Instruction::Call { arguments, .. } => { + for arg in arguments { + if self.inserter.function.dfg.value_is_reference(*arg) { + self.stores_used_in_calls + .entry(*arg) + .or_default() + .push((instruction, block_id)); + } + } + self.mark_all_unknown(arguments, references); + } _ => (), } } @@ -415,7 +597,17 @@ impl<'f> PerFunctionContext<'f> { fn handle_terminator(&mut self, block: BasicBlockId, references: &mut Block) { self.inserter.map_terminator_in_place(block); - match self.inserter.function.dfg[block].unwrap_terminator() { + let terminator = self.inserter.function.dfg[block].unwrap_terminator(); + + terminator.for_each_value(|value| { + if let Some(PerFuncLoadResultContext { load_counter, .. }) = + self.load_results.get_mut(&value) + { + *load_counter += 1; + } + }); + + match terminator { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do TerminatorInstruction::Jmp { destination, arguments, .. } => { let destination_parameters = self.inserter.function.dfg[*destination].parameters(); From 260acd0b55bd310076202fd4e3a74214e46038b1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 18:52:22 +0000 Subject: [PATCH 02/58] delete rc load tracker --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index b63c3f3c7b0..096398a179d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -123,25 +123,6 @@ struct PerFunctionContext<'f> { /// Track whether a reference was passed into another entry point stores_used_in_calls: HashMap>, - - /// Flag for tracking whether we had to perform a re-load as part of the Brillig CoW optimization. - /// Stores made as part of this optimization should not be removed. - /// We want to catch stores of this nature: - /// ```text - /// v3 = load v1 - // inc_rc v3 - // v4 = load v1 - // inc_rc v4 - // store v4 at v1 - // store v3 at v2 - /// ``` - /// - /// We keep track of an optional boolean flag as we go through instructions. - /// If the flag exists it means we have hit a load instruction. - /// If the flag is false it means we have processed a single load, while if the flag is true - /// it means we have performed a re-load. - /// The field is reset to `None` on every instruction that is not a load, inc_rc, dec_rc, or function call. - inside_rc_reload: Option, } #[derive(Debug, Clone)] @@ -170,7 +151,6 @@ impl<'f> PerFunctionContext<'f> { instructions_to_remove: BTreeSet::new(), last_loads: HashMap::default(), load_results: HashMap::default(), - inside_rc_reload: None, stores_used_in_calls: HashMap::default(), } } From 0c575fa87059f8dad1d430185ac6d29430439478 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 21:13:43 +0000 Subject: [PATCH 03/58] add test and separate logic into cleaner functions --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 481 ++++++++++++------ noir_stdlib/src/hash/mod.nr | 2 +- 2 files changed, 333 insertions(+), 150 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 096398a179d..b2a957f109d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -66,7 +66,7 @@ mod block; use std::collections::{BTreeMap, BTreeSet}; -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::ssa::{ ir::{ @@ -122,7 +122,8 @@ struct PerFunctionContext<'f> { load_results: HashMap, /// Track whether a reference was passed into another entry point - stores_used_in_calls: HashMap>, + /// This is needed to determine whether we can remove a store. + calls_reference_input: HashSet, } #[derive(Debug, Clone)] @@ -151,7 +152,7 @@ impl<'f> PerFunctionContext<'f> { instructions_to_remove: BTreeSet::new(), last_loads: HashMap::default(), load_results: HashMap::default(), - stores_used_in_calls: HashMap::default(), + calls_reference_input: HashSet::default(), } } @@ -169,148 +170,8 @@ impl<'f> PerFunctionContext<'f> { self.analyze_block(block, references); } - let mut loads_removed = HashMap::default(); - for (_, PerFuncLoadResultContext { load_counter, load_instruction, .. }) in - self.load_results.iter() - { - let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] - else { - panic!("Should only have a load instruction here"); - }; - - if *load_counter == 0 { - if let Some(counter) = loads_removed.get_mut(&address) { - *counter += 1; - } else { - loads_removed.insert(address, 1); - } - - self.instructions_to_remove.insert(*load_instruction); - } - } - - let mut not_removed_stores: HashMap = HashMap::default(); - // 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_id, block) in self.blocks.iter() { - let block_params = self.inserter.function.dfg.block_parameters(*block_id); - for (store_address, store_instruction) in block.last_stores.iter() { - let is_reference_param = block_params.contains(store_address); - let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); - - let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); - let remove_load = if is_return { - // Determine whether the last store is used in the return value - let mut is_return_value = false; - terminator.for_each_value(|return_value| { - is_return_value = return_value == *store_address || is_return_value; - }); - - // If the last load of a store is not part of the block with a return terminator, - // we can safely remove this store. - let last_load_not_in_return = self - .last_loads - .get(store_address) - .map(|(_, last_load_block, _)| *last_load_block != *block_id) - .unwrap_or(true); - !is_return_value && last_load_not_in_return - } else if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = - (self.last_loads.get(store_address), loads_removed.get(store_address)) - { - *last_loads_counter == *loads_removed_counter - } else { - self.last_loads.get(store_address).is_none() - }; - - let is_not_used_in_reference_param = - self.stores_used_in_calls.get(store_address).is_none(); - if remove_load && !is_reference_param && is_not_used_in_reference_param { - self.instructions_to_remove.insert(*store_instruction); - if let Some((_, counter)) = not_removed_stores.get_mut(store_address) { - *counter -= 1; - } - } else if let Some((_, counter)) = not_removed_stores.get_mut(store_address) { - *counter += 1; - } else { - not_removed_stores.insert(*store_address, (*store_instruction, 1)); - } - } - } - - self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, .. }| { - let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] - else { - panic!("Should only have a load instruction here"); - }; - not_removed_stores.contains_key(&address) - }); - - let mut new_instructions = HashMap::default(); - for (store_address, (store_instruction, store_counter)) in not_removed_stores { - let Instruction::Store { value, .. } = self.inserter.function.dfg[store_instruction] - else { - panic!("Should only have a store instruction"); - }; - - if store_counter != 0 { - continue; - } - self.instructions_to_remove.insert(store_instruction); - - if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = - (self.last_loads.get(&store_address), loads_removed.get(&store_address)) - { - if *last_loads_counter < *loads_removed_counter { - panic!("The number of loads removed should not be more than all loads"); - } - } - - for ( - result, - PerFuncLoadResultContext { - load_counter, - load_instruction, - instructions_using_result, - }, - ) in self.load_results.iter() - { - let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] - else { - panic!("Should only have a load instruction here"); - }; - if address != store_address { - continue; - } - - if *load_counter > 0 { - self.inserter.map_value(*result, value); - for (instruction, block_id) in instructions_using_result { - let new_instruction = - self.inserter.push_instruction(*instruction, *block_id); - if let Some(new_instruction) = new_instruction { - new_instructions - .insert((*instruction, block_id), Some(new_instruction)); - } else { - new_instructions.insert((*instruction, block_id), None); - } - } - - self.instructions_to_remove.insert(*load_instruction); - } - } - } - - // Re-assign or delete any mapped instructions after the final loads were removed. - for ((old_instruction, block_id), new_instruction) in new_instructions { - let instructions = self.inserter.function.dfg[*block_id].instructions_mut(); - if let Some(index) = instructions.iter().position(|v| *v == old_instruction) { - if let Some(new_instruction) = new_instruction { - instructions[index] = new_instruction; - } else { - instructions.remove(index); - } - } - } + self.cleanup_function(); + // self.cleanup_function(); } /// The value of each reference at the start of the given block is the unification @@ -502,10 +363,7 @@ impl<'f> PerFunctionContext<'f> { Instruction::Call { arguments, .. } => { for arg in arguments { if self.inserter.function.dfg.value_is_reference(*arg) { - self.stores_used_in_calls - .entry(*arg) - .or_default() - .push((instruction, block_id)); + self.calls_reference_input.insert(*arg); } } self.mark_all_unknown(arguments, references); @@ -615,6 +473,174 @@ impl<'f> PerFunctionContext<'f> { } } } + + /// The mem2reg pass is sometimes unable to determine certain known values + /// when iterating over a function's block in reverse post order. + /// We collect state about any final loads and stores to a given address during the initial mem2reg pass. + /// We can then utilize this state to clean up any loads and stores that may have been missed. + fn cleanup_function(&mut self) { + let removed_loads = self.remove_unused_loads(); + let remaining_last_stores = self.remove_unloaded_last_stores(&removed_loads); + self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); + } + + /// Cleanup remaining loads across the entire function + /// Remove any loads whose reference counter is zero. + /// Returns a map of the removed load address to the number of load instructions removed for that address + fn remove_unused_loads(&mut self) -> HashMap { + let mut removed_loads = HashMap::default(); + for (_, PerFuncLoadResultContext { load_counter, load_instruction, .. }) in + self.load_results.iter() + { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + panic!("Should only have a load instruction here"); + }; + + if *load_counter == 0 { + if let Some(counter) = removed_loads.get_mut(&address) { + *counter += 1; + } else { + removed_loads.insert(address, 1); + } + + self.instructions_to_remove.insert(*load_instruction); + } + } + removed_loads + } + + /// Cleanup remaining stores across the entire function. + /// 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. + /// Returns a map of any remaining stores which may still have loads in use. + fn remove_unloaded_last_stores( + &mut self, + removed_loads: &HashMap, + ) -> HashMap { + let mut remaining_last_stores: HashMap = HashMap::default(); + for (block_id, block) in self.blocks.iter() { + let block_params = self.inserter.function.dfg.block_parameters(*block_id); + for (store_address, store_instruction) in block.last_stores.iter() { + let is_reference_param = block_params.contains(store_address); + let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); + + let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); + // Determine whether any loads that reference this store address + // have been removed while cleaning up unused loads. + let remove_load = if is_return { + // Determine whether the last store is used in the return value + let mut is_return_value = false; + terminator.for_each_value(|return_value| { + is_return_value = return_value == *store_address || is_return_value; + }); + + // If the last load of a store is not part of the block with a return terminator, + // we can safely remove this store. + let last_load_not_in_return = self + .last_loads + .get(store_address) + .map(|(_, last_load_block, _)| *last_load_block != *block_id) + .unwrap_or(true); + !is_return_value && last_load_not_in_return + } else if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = + (self.last_loads.get(store_address), removed_loads.get(store_address)) + { + // `last_loads` contains the total number of loads for a given load address + // If the number of loads removed + *last_loads_counter == *loads_removed_counter + } else { + // Otherwise just check whether a load exists at all for this store address + self.last_loads.get(store_address).is_none() + }; + + let is_not_used_in_reference_param = + self.calls_reference_input.get(store_address).is_none(); + if remove_load && !is_reference_param && is_not_used_in_reference_param { + self.instructions_to_remove.insert(*store_instruction); + if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { + *counter -= 1; + } + // else { + // remaining_last_stores.insert(*store_address, (*store_instruction, 1)); + // } + } else if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { + *counter += 1; + } else { + remaining_last_stores.insert(*store_address, (*store_instruction, 1)); + } + } + } + remaining_last_stores + } + + /// Check if any remaining last stores are only used in a single load + fn remove_remaining_last_stores( + &mut self, + removed_loads: &HashMap, + remaining_last_stores: &HashMap, + ) { + // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores + self.load_results.retain( + |_, PerFuncLoadResultContext { load_instruction, load_counter, .. }| { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + panic!("Should only have a load instruction here"); + }; + remaining_last_stores.contains_key(&address) && *load_counter > 0 + }, + ); + + for (store_address, (store_instruction, store_counter)) in remaining_last_stores { + let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] + else { + panic!("Should only have a store instruction"); + }; + + if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = + (self.last_loads.get(store_address), removed_loads.get(store_address)) + { + if *last_loads_counter < *loads_removed_counter { + panic!("The number of loads removed should not be more than all loads"); + } + } + + if *store_counter != 0 { + continue; + } + + self.instructions_to_remove.insert(*store_instruction); + + // Map any remaining load results to the value from the removed store + for ( + result, + PerFuncLoadResultContext { load_instruction, instructions_using_result, .. }, + ) in self.load_results.iter() + { + self.inserter.map_value(*result, value); + for (instruction, block_id) in instructions_using_result { + if self.instructions_to_remove.contains(instruction) { + continue; + } + + let new_instruction = self.inserter.push_instruction(*instruction, *block_id); + + let instructions = self.inserter.function.dfg[*block_id].instructions_mut(); + + // Re-assign or delete any mapped instructions after the final loads were removed. + if let Some(index) = instructions.iter().position(|v| *v == *instruction) { + if let Some(new_instruction) = new_instruction { + instructions[index] = new_instruction; + } else { + instructions.remove(index); + } + } + } + + self.instructions_to_remove.insert(*load_instruction); + } + } + } } #[cfg(test)] @@ -921,4 +947,161 @@ mod tests { // We expect the last eq to be optimized out assert_eq!(b1_instructions.len(), 0); } + + #[test] + fn remove_unused_loads_and_stores() { + // acir(inline) fn main f0 { + // b0(): + // v0 = allocate + // store Field 1 at v0 + // v2 = allocate + // store Field 1 at v2 + // v4 = allocate + // store u1 0 at v4 + // v5 = allocate + // store u1 0 at v5 + // v6 = allocate + // store u1 0 at v6 + // jmp b1(u1 0) + // b1(v7: u32): + // v9 = eq v7, u32 0 + // jmpif v9 then: b3, else: b2 + // b3(): + // v20 = load v0 + // v21 = load v2 + // v22 = load v4 + // v23 = load v5 + // v24 = load v6 + // constrain v20 == Field 1 + // v25 = eq v21, Field 1 + // constrain v21 == Field 1 + // v26 = eq v7, u32 0 + // jmp b1(v26) + // b2(): + // v10 = load v0 + // v11 = load v2 + // v12 = load v4 + // v13 = load v5 + // v14 = load v6 + // store Field 1 at v0 + // store Field 1 at v2 + // store v12 at v4 + // store v13 at v5 + // store v14 at v6 + // v15 = load v0 + // v16 = load v2 + // v17 = load v4 + // v18 = load v5 + // v19 = load v6 + // constrain v15 == Field 1 + // return v16 + // } + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + + let v0 = builder.insert_allocate(Type::field()); + let one = builder.numeric_constant(1u128, Type::field()); + builder.insert_store(v0, one); + + let v2 = builder.insert_allocate(Type::field()); + builder.insert_store(v2, one); + + let zero_bool = builder.numeric_constant(0u128, Type::bool()); + let v4 = builder.insert_allocate(Type::bool()); + builder.insert_store(v4, zero_bool); + + let v6 = builder.insert_allocate(Type::bool()); + builder.insert_store(v6, zero_bool); + + let v8 = builder.insert_allocate(Type::bool()); + builder.insert_store(v8, zero_bool); + + let b1 = builder.insert_block(); + builder.terminate_with_jmp(b1, vec![zero_bool]); + + builder.switch_to_block(b1); + + let v7 = builder.add_block_parameter(b1, Type::unsigned(32)); + let zero_u32 = builder.numeric_constant(0u128, Type::unsigned(32)); + let is_zero = builder.insert_binary(v7, BinaryOp::Eq, zero_u32); + + let b2 = builder.insert_block(); + let b3 = builder.insert_block(); + builder.terminate_with_jmpif(is_zero, b3, b2); + + builder.switch_to_block(b2); + + let _ = builder.insert_load(v0, Type::field()); + let _ = builder.insert_load(v2, Type::field()); + let v12 = builder.insert_load(v4, Type::bool()); + let v13 = builder.insert_load(v6, Type::bool()); + let v14 = builder.insert_load(v8, Type::bool()); + + builder.insert_store(v0, one); + builder.insert_store(v2, one); + builder.insert_store(v4, v12); + builder.insert_store(v6, v13); + builder.insert_store(v8, v14); + + let v15 = builder.insert_load(v0, Type::field()); + // Insert unused loads + let v16 = builder.insert_load(v2, Type::field()); + let _ = builder.insert_load(v4, Type::bool()); + let _ = builder.insert_load(v6, Type::bool()); + let _ = builder.insert_load(v8, Type::bool()); + + builder.insert_constrain(v15, one, None); + builder.terminate_with_return(vec![v16]); + + builder.switch_to_block(b3); + + let v26 = builder.insert_load(v0, Type::field()); + // Insert unused loads + let v27 = builder.insert_load(v2, Type::field()); + let _ = builder.insert_load(v4, Type::bool()); + let _ = builder.insert_load(v6, Type::bool()); + let _ = builder.insert_load(v8, Type::bool()); + + builder.insert_constrain(v26, one, None); + let _ = builder.insert_binary(v27, BinaryOp::Eq, one); + builder.insert_constrain(v27, one, None); + let one_u32 = builder.numeric_constant(0u128, Type::unsigned(32)); + let plus_one = builder.insert_binary(v7, BinaryOp::Eq, one_u32); + builder.terminate_with_jmp(b1, vec![plus_one]); + + let ssa = builder.finish(); + println!("{}", ssa); + + // Expected result: + // acir(inline) fn main f0 { + // b0(): + // v27 = allocate + // v28 = allocate + // v29 = allocate + // v30 = allocate + // v31 = allocate + // jmp b1(u1 0) + // b1(v7: u32): + // v32 = eq v7, u32 0 + // jmpif v32 then: b3, else: b2 + // b3(): + // v49 = eq v7, u32 0 + // jmp b1(v49) + // b2(): + // return Field 1 + // } + let ssa = ssa.mem2reg(); + println!("{}", ssa); + + let main = ssa.main(); + assert_eq!(main.reachable_blocks().len(), 4); + + // All loads should be removed + assert_eq!(count_loads(b2, &main.dfg), 0); + assert_eq!(count_loads(b3, &main.dfg), 0); + + // All stores should be removed + assert_eq!(count_stores(main.entry_block(), &main.dfg), 0); + assert_eq!(count_stores(b2, &main.dfg), 0); + } } diff --git a/noir_stdlib/src/hash/mod.nr b/noir_stdlib/src/hash/mod.nr index 0e15595ff40..33be56cdc3d 100644 --- a/noir_stdlib/src/hash/mod.nr +++ b/noir_stdlib/src/hash/mod.nr @@ -12,7 +12,7 @@ use crate::embedded_curve_ops::{EmbeddedCurvePoint, EmbeddedCurveScalar, multi_s use crate::meta::derive_via; // Kept for backwards compatibility -pub use sha256::{digest, sha256, sha256_compression, sha256_var}; +use sha256::{digest, sha256, sha256_compression, sha256_var}; #[foreign(blake2s)] // docs:start:blake2s From 409fbff607097ab19326d7fa8bb738158970e052 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 21:15:53 +0000 Subject: [PATCH 04/58] clenaup --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 4 ---- noir_stdlib/src/hash/mod.nr | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index b2a957f109d..2a420e33c40 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -171,7 +171,6 @@ impl<'f> PerFunctionContext<'f> { } self.cleanup_function(); - // self.cleanup_function(); } /// The value of each reference at the start of the given block is the unification @@ -561,9 +560,6 @@ impl<'f> PerFunctionContext<'f> { if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; } - // else { - // remaining_last_stores.insert(*store_address, (*store_instruction, 1)); - // } } else if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter += 1; } else { diff --git a/noir_stdlib/src/hash/mod.nr b/noir_stdlib/src/hash/mod.nr index 33be56cdc3d..0e15595ff40 100644 --- a/noir_stdlib/src/hash/mod.nr +++ b/noir_stdlib/src/hash/mod.nr @@ -12,7 +12,7 @@ use crate::embedded_curve_ops::{EmbeddedCurvePoint, EmbeddedCurveScalar, multi_s use crate::meta::derive_via; // Kept for backwards compatibility -use sha256::{digest, sha256, sha256_compression, sha256_var}; +pub use sha256::{digest, sha256, sha256_compression, sha256_var}; #[foreign(blake2s)] // docs:start:blake2s From 00226392ed7d586386786ed1bc903f1a4706660c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 21:27:13 +0000 Subject: [PATCH 05/58] update comment --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 2a420e33c40..299f752f759 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -546,7 +546,8 @@ impl<'f> PerFunctionContext<'f> { (self.last_loads.get(store_address), removed_loads.get(store_address)) { // `last_loads` contains the total number of loads for a given load address - // If the number of loads removed + // If the number of loads removed is equal to the total number of loads for an address, + // we know we can safely remove any stores to that load address. *last_loads_counter == *loads_removed_counter } else { // Otherwise just check whether a load exists at all for this store address From 825b9788fd6db9652bbfbed6ac2e5b009db07d88 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 21:31:38 +0000 Subject: [PATCH 06/58] another comment improvement --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 299f752f759..f01459f596a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -546,7 +546,7 @@ impl<'f> PerFunctionContext<'f> { (self.last_loads.get(store_address), removed_loads.get(store_address)) { // `last_loads` contains the total number of loads for a given load address - // If the number of loads removed is equal to the total number of loads for an address, + // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. *last_loads_counter == *loads_removed_counter } else { From 2856501fc9124237fe6ee4d0dc4a02bb541a619d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 21:51:12 +0000 Subject: [PATCH 07/58] add back check that store address equals the load result address for the instruction we are mapping --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 48 ++++++++++++++----- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index f01459f596a..f4efe0df235 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -128,14 +128,17 @@ struct PerFunctionContext<'f> { #[derive(Debug, Clone)] struct PerFuncLoadResultContext { - load_counter: u32, + // Reference counter that keeps track of how many times a load was used in other instructions + result_counter: u32, + // Load instruction that produced a given load result load_instruction: InstructionId, + // Instructions that use a given load result instructions_using_result: Vec<(InstructionId, BasicBlockId)>, } impl PerFuncLoadResultContext { fn new(load_instruction: InstructionId) -> Self { - Self { load_counter: 0, load_instruction, instructions_using_result: vec![] } + Self { result_counter: 0, load_instruction, instructions_using_result: vec![] } } } @@ -256,9 +259,12 @@ impl<'f> PerFunctionContext<'f> { return; } + // Track whether any load results were used in the instruction self.inserter.function.dfg[instruction].for_each_value(|value| { if let Some(PerFuncLoadResultContext { - load_counter, instructions_using_result, .. + result_counter: load_counter, + instructions_using_result, + .. }) = self.load_results.get_mut(&value) { *load_counter += 1; @@ -301,10 +307,10 @@ impl<'f> PerFunctionContext<'f> { // function calls in-between, we can remove the previous store. if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); - if let Some(PerFuncLoadResultContext { load_counter, .. }) = + if let Some(PerFuncLoadResultContext { result_counter, .. }) = self.load_results.get_mut(&value) { - *load_counter -= 1; + *result_counter -= 1; } } @@ -437,10 +443,10 @@ impl<'f> PerFunctionContext<'f> { let terminator = self.inserter.function.dfg[block].unwrap_terminator(); terminator.for_each_value(|value| { - if let Some(PerFuncLoadResultContext { load_counter, .. }) = + if let Some(PerFuncLoadResultContext { result_counter, .. }) = self.load_results.get_mut(&value) { - *load_counter += 1; + *result_counter += 1; } }); @@ -478,6 +484,8 @@ impl<'f> PerFunctionContext<'f> { /// We collect state about any final loads and stores to a given address during the initial mem2reg pass. /// We can then utilize this state to clean up any loads and stores that may have been missed. fn cleanup_function(&mut self) { + // Removing remaining unused loads during mem2reg can help expose removable stores that the initial + // mem2reg pass deemed we could not remove due to the existence of those unused loads. let removed_loads = self.remove_unused_loads(); let remaining_last_stores = self.remove_unloaded_last_stores(&removed_loads); self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); @@ -488,7 +496,7 @@ impl<'f> PerFunctionContext<'f> { /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { let mut removed_loads = HashMap::default(); - for (_, PerFuncLoadResultContext { load_counter, load_instruction, .. }) in + for (_, PerFuncLoadResultContext { result_counter, load_instruction, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] @@ -496,7 +504,8 @@ impl<'f> PerFunctionContext<'f> { panic!("Should only have a load instruction here"); }; - if *load_counter == 0 { + // If the load result's counter is equal to zero we can safely remove that load instruction. + if *result_counter == 0 { if let Some(counter) = removed_loads.get_mut(&address) { *counter += 1; } else { @@ -579,12 +588,12 @@ impl<'f> PerFunctionContext<'f> { ) { // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores self.load_results.retain( - |_, PerFuncLoadResultContext { load_instruction, load_counter, .. }| { + |_, PerFuncLoadResultContext { load_instruction, result_counter, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { panic!("Should only have a load instruction here"); }; - remaining_last_stores.contains_key(&address) && *load_counter > 0 + remaining_last_stores.contains_key(&address) && *result_counter > 0 }, ); @@ -602,6 +611,7 @@ impl<'f> PerFunctionContext<'f> { } } + // We only want to remove stores if *store_counter != 0 { continue; } @@ -611,9 +621,21 @@ impl<'f> PerFunctionContext<'f> { // Map any remaining load results to the value from the removed store for ( result, - PerFuncLoadResultContext { load_instruction, instructions_using_result, .. }, + PerFuncLoadResultContext { + load_instruction, + instructions_using_result, + .. + }, ) in self.load_results.iter() { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + panic!("Should only have a load instruction here"); + }; + if address != *store_address { + continue; + } + self.inserter.map_value(*result, value); for (instruction, block_id) in instructions_using_result { if self.instructions_to_remove.contains(instruction) { @@ -1100,5 +1122,7 @@ mod tests { // All stores should be removed assert_eq!(count_stores(main.entry_block(), &main.dfg), 0); assert_eq!(count_stores(b2, &main.dfg), 0); + // Should only have one instruction in b3 + assert_eq!(main.dfg[b3].instructions().len(), 1); } } From 8473816ca3137aecb86ab3aef1350bfb9c208dd4 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 21:56:00 +0000 Subject: [PATCH 08/58] cargo fmt --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index f4efe0df235..15f421b1124 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -621,11 +621,7 @@ impl<'f> PerFunctionContext<'f> { // Map any remaining load results to the value from the removed store for ( result, - PerFuncLoadResultContext { - load_instruction, - instructions_using_result, - .. - }, + PerFuncLoadResultContext { load_instruction, instructions_using_result, .. }, ) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] From 5636a527e53d2950b22c8fd48f0f333d7c217711 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:05:54 +0100 Subject: [PATCH 09/58] change some panics to unreachables --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 15f421b1124..1642fd0f29a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -501,7 +501,7 @@ impl<'f> PerFunctionContext<'f> { { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { - panic!("Should only have a load instruction here"); + unreachable!("Should only have a load instruction here"); }; // If the load result's counter is equal to zero we can safely remove that load instruction. @@ -600,15 +600,16 @@ impl<'f> PerFunctionContext<'f> { for (store_address, (store_instruction, store_counter)) in remaining_last_stores { let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] else { - panic!("Should only have a store instruction"); + unreachable!("Should only have a store instruction"); }; if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = (self.last_loads.get(store_address), removed_loads.get(store_address)) { - if *last_loads_counter < *loads_removed_counter { - panic!("The number of loads removed should not be more than all loads"); - } + assert!( + *last_loads_counter >= *loads_removed_counter, + "The number of loads removed should not be more than all loads" + ); } // We only want to remove stores @@ -626,7 +627,7 @@ impl<'f> PerFunctionContext<'f> { { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { - panic!("Should only have a load instruction here"); + unreachable!("Should only have a load instruction here"); }; if address != *store_address { continue; From 60448ced4839c84c157a90abda6fea9c1d8128ff Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:06:12 +0100 Subject: [PATCH 10/58] doc comment --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 1642fd0f29a..0c101a48a6e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -128,11 +128,11 @@ struct PerFunctionContext<'f> { #[derive(Debug, Clone)] struct PerFuncLoadResultContext { - // Reference counter that keeps track of how many times a load was used in other instructions + /// Reference counter that keeps track of how many times a load was used in other instructions result_counter: u32, - // Load instruction that produced a given load result + /// Load instruction that produced a given load result load_instruction: InstructionId, - // Instructions that use a given load result + /// Instructions that use a given load result instructions_using_result: Vec<(InstructionId, BasicBlockId)>, } From e2ee0049a96da53e759c6534d1a23512266e73de Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:06:34 +0100 Subject: [PATCH 11/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 0c101a48a6e..e8ac922decc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -591,7 +591,7 @@ impl<'f> PerFunctionContext<'f> { |_, PerFuncLoadResultContext { load_instruction, result_counter, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { - panic!("Should only have a load instruction here"); + unreachable!("Should only have a load instruction here"); }; remaining_last_stores.contains_key(&address) && *result_counter > 0 }, From 5eb0cbc836a3bf735b645fe2fc7dc963ba001079 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 11:38:47 -0400 Subject: [PATCH 12/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e8ac922decc..e7a8817c707 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -288,12 +288,10 @@ impl<'f> PerFunctionContext<'f> { self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); - let load_counter = - if let Some((_, _, load_counter)) = self.last_loads.get(&address) { - *load_counter + 1 - } else { - 1 - }; + let load_counter = self + .last_loads + .get(&address) + .map_or(1, |(_, _, load_counter)| *load_counter + 1); self.last_loads.insert(address, (instruction, block_id, load_counter)); } } From 50dd6bcf49d960721fca605d52e5b00a813632fd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 11:41:20 -0400 Subject: [PATCH 13/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e7a8817c707..e3cd5355573 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -504,11 +504,7 @@ impl<'f> PerFunctionContext<'f> { // If the load result's counter is equal to zero we can safely remove that load instruction. if *result_counter == 0 { - if let Some(counter) = removed_loads.get_mut(&address) { - *counter += 1; - } else { - removed_loads.insert(address, 1); - } + removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); self.instructions_to_remove.insert(*load_instruction); } From 54fa3d345a6cf5404f5c997ab1ffd265ead46983 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 11:41:33 -0400 Subject: [PATCH 14/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e3cd5355573..767928ae101 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -261,14 +261,9 @@ impl<'f> PerFunctionContext<'f> { // Track whether any load results were used in the instruction self.inserter.function.dfg[instruction].for_each_value(|value| { - if let Some(PerFuncLoadResultContext { - result_counter: load_counter, - instructions_using_result, - .. - }) = self.load_results.get_mut(&value) - { - *load_counter += 1; - instructions_using_result.push((instruction, block_id)); + if let Some(context) = self.load_results.get_mut(&value) { + context.result_counter += 1; + context.instructions_using_result.push((instruction, block_id)); } }); From d438f945d8e6afc3bbd28f52957de2ba1ecf0b73 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 11:41:45 -0400 Subject: [PATCH 15/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 767928ae101..e401f472b4b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -436,10 +436,8 @@ impl<'f> PerFunctionContext<'f> { let terminator = self.inserter.function.dfg[block].unwrap_terminator(); terminator.for_each_value(|value| { - if let Some(PerFuncLoadResultContext { result_counter, .. }) = - self.load_results.get_mut(&value) - { - *result_counter += 1; + if let Some(context) = self.load_results.get_mut(&value) { + context.result_counter += 1; } }); From 84c0fc15e73348e70ae08f2a25bf88dbed240a52 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 11:41:51 -0400 Subject: [PATCH 16/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e401f472b4b..e3df4750abd 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -300,10 +300,8 @@ impl<'f> PerFunctionContext<'f> { // function calls in-between, we can remove the previous store. if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); - if let Some(PerFuncLoadResultContext { result_counter, .. }) = - self.load_results.get_mut(&value) - { - *result_counter -= 1; + if let Some(context) = self.load_results.get_mut(&value) { + context.result_counter -= 1; } } From f35f3a22b65a30f85b9023b88e5885e05d57b9e2 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 15:42:45 +0000 Subject: [PATCH 17/58] cargo fmt --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e3df4750abd..b519f90885d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -283,7 +283,7 @@ impl<'f> PerFunctionContext<'f> { self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); - let load_counter = self + let load_counter = self .last_loads .get(&address) .map_or(1, |(_, _, load_counter)| *load_counter + 1); From aff05116828a1791e658c47db3ab67f5c25e32ac Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 16:07:37 +0000 Subject: [PATCH 18/58] fixup comment --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index b519f90885d..187114a1d0b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -597,7 +597,7 @@ impl<'f> PerFunctionContext<'f> { ); } - // We only want to remove stores + // We only want to remove last stores referencing a single address. if *store_counter != 0 { continue; } From a6f53253aad6024f105e0e59da3ea66d1937cd3e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 16:08:50 +0000 Subject: [PATCH 19/58] do not restructure in the final load results loop' --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 187114a1d0b..ceb2efa9253 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -605,12 +605,9 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*store_instruction); // Map any remaining load results to the value from the removed store - for ( - result, - PerFuncLoadResultContext { load_instruction, instructions_using_result, .. }, - ) in self.load_results.iter() - { - let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + for (result, context) in self.load_results.iter() { + let Instruction::Load { address } = + self.inserter.function.dfg[context.load_instruction] else { unreachable!("Should only have a load instruction here"); }; @@ -619,7 +616,7 @@ impl<'f> PerFunctionContext<'f> { } self.inserter.map_value(*result, value); - for (instruction, block_id) in instructions_using_result { + for (instruction, block_id) in &context.instructions_using_result { if self.instructions_to_remove.contains(instruction) { continue; } @@ -638,7 +635,7 @@ impl<'f> PerFunctionContext<'f> { } } - self.instructions_to_remove.insert(*load_instruction); + self.instructions_to_remove.insert(context.load_instruction); } } } From ce972bcb7d9f173cf905d6729ff2d279b360e953 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 16:53:09 +0000 Subject: [PATCH 20/58] result_counter -> uses --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 1dda2a3a608..883b0199dfb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -133,7 +133,7 @@ struct PerFunctionContext<'f> { #[derive(Debug, Clone)] struct PerFuncLoadResultContext { /// Reference counter that keeps track of how many times a load was used in other instructions - result_counter: u32, + uses: u32, /// Load instruction that produced a given load result load_instruction: InstructionId, /// Instructions that use a given load result @@ -142,7 +142,7 @@ struct PerFuncLoadResultContext { impl PerFuncLoadResultContext { fn new(load_instruction: InstructionId) -> Self { - Self { result_counter: 0, load_instruction, instructions_using_result: vec![] } + Self { uses: 0, load_instruction, instructions_using_result: vec![] } } } @@ -267,7 +267,7 @@ impl<'f> PerFunctionContext<'f> { // Track whether any load results were used in the instruction self.inserter.function.dfg[instruction].for_each_value(|value| { if let Some(context) = self.load_results.get_mut(&value) { - context.result_counter += 1; + context.uses += 1; context.instructions_using_result.push((instruction, block_id)); } }); @@ -310,7 +310,7 @@ impl<'f> PerFunctionContext<'f> { if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); if let Some(context) = self.load_results.get_mut(&value) { - context.result_counter -= 1; + context.uses -= 1; } } @@ -464,7 +464,7 @@ impl<'f> PerFunctionContext<'f> { terminator.for_each_value(|value| { if let Some(context) = self.load_results.get_mut(&value) { - context.result_counter += 1; + context.uses += 1; } }); @@ -514,8 +514,7 @@ impl<'f> PerFunctionContext<'f> { /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { let mut removed_loads = HashMap::default(); - for (_, PerFuncLoadResultContext { result_counter, load_instruction, .. }) in - self.load_results.iter() + for (_, PerFuncLoadResultContext { uses, load_instruction, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { @@ -523,7 +522,7 @@ impl<'f> PerFunctionContext<'f> { }; // If the load result's counter is equal to zero we can safely remove that load instruction. - if *result_counter == 0 { + if *uses == 0 { removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); self.instructions_to_remove.insert(*load_instruction); @@ -601,15 +600,13 @@ impl<'f> PerFunctionContext<'f> { remaining_last_stores: &HashMap, ) { // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores - self.load_results.retain( - |_, PerFuncLoadResultContext { load_instruction, result_counter, .. }| { - let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] - else { - unreachable!("Should only have a load instruction here"); - }; - remaining_last_stores.contains_key(&address) && *result_counter > 0 - }, - ); + self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, uses, .. }| { + let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] + else { + unreachable!("Should only have a load instruction here"); + }; + remaining_last_stores.contains_key(&address) && *uses > 0 + }); for (store_address, (store_instruction, store_counter)) in remaining_last_stores { let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] From ac9c1d2d8a76a48b0d637193985bca5e2aba952d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 12:53:42 -0400 Subject: [PATCH 21/58] Update compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs Co-authored-by: jfecher --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 883b0199dfb..87330a649d1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -292,11 +292,11 @@ impl<'f> PerFunctionContext<'f> { self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); - let load_counter = self + let last_load = self .last_loads - .get(&address) - .map_or(1, |(_, _, load_counter)| *load_counter + 1); - self.last_loads.insert(address, (instruction, block_id, load_counter)); + .entry(address) + .or_insert((instruction, block_id, 0)); + last_load.load_counter += 1; } } Instruction::Store { address, value } => { From 2bc7b8b4e470d18afb1bd52b424fd5d924e3bb78 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 16:54:11 +0000 Subject: [PATCH 22/58] fmt --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 87330a649d1..5865657e6a2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -292,10 +292,8 @@ impl<'f> PerFunctionContext<'f> { self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); - let last_load = self - .last_loads - .entry(address) - .or_insert((instruction, block_id, 0)); + let last_load = + self.last_loads.entry(address).or_insert((instruction, block_id, 0)); last_load.load_counter += 1; } } From 399e6d9a20d407321e92d205bf3d8d9c530cdb96 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 17:01:59 +0000 Subject: [PATCH 23/58] PerFuncLastLoadContext --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 36 ++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 5865657e6a2..993407f6bd6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -116,7 +116,7 @@ struct PerFunctionContext<'f> { /// 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, + last_loads: HashMap, /// Track whether a load result was used across all blocks. load_results: HashMap, @@ -130,6 +130,22 @@ struct PerFunctionContext<'f> { inside_rc_reload: bool, } +#[derive(Debug, Clone)] +struct PerFuncLastLoadContext { + /// Reference counter that keeps track of how many times we loaded from a given address + num_loads: u32, + /// Last load instruction from a given address + load_instruction: InstructionId, + /// Block of the last load instruction + block_id: BasicBlockId, +} + +impl PerFuncLastLoadContext { + fn new(load_instruction: InstructionId, block_id: BasicBlockId) -> Self { + Self { num_loads: 0, load_instruction, block_id } + } +} + #[derive(Debug, Clone)] struct PerFuncLoadResultContext { /// Reference counter that keeps track of how many times a load was used in other instructions @@ -292,9 +308,11 @@ impl<'f> PerFunctionContext<'f> { self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); - let last_load = - self.last_loads.entry(address).or_insert((instruction, block_id, 0)); - last_load.load_counter += 1; + let last_load = self + .last_loads + .entry(address) + .or_insert(PerFuncLastLoadContext::new(instruction, block_id)); + last_load.num_loads += 1; } } Instruction::Store { address, value } => { @@ -559,16 +577,16 @@ impl<'f> PerFunctionContext<'f> { let last_load_not_in_return = self .last_loads .get(store_address) - .map(|(_, last_load_block, _)| *last_load_block != *block_id) + .map(|context| context.block_id != *block_id) .unwrap_or(true); !is_return_value && last_load_not_in_return - } else if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = + } else if let (Some(context), Some(loads_removed_counter)) = (self.last_loads.get(store_address), removed_loads.get(store_address)) { // `last_loads` contains the total number of loads for a given load address // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. - *last_loads_counter == *loads_removed_counter + context.num_loads == *loads_removed_counter } else { // Otherwise just check whether a load exists at all for this store address self.last_loads.get(store_address).is_none() @@ -612,11 +630,11 @@ impl<'f> PerFunctionContext<'f> { unreachable!("Should only have a store instruction"); }; - if let (Some((_, _, last_loads_counter)), Some(loads_removed_counter)) = + if let (Some(context), Some(loads_removed_counter)) = (self.last_loads.get(store_address), removed_loads.get(store_address)) { assert!( - *last_loads_counter >= *loads_removed_counter, + context.num_loads >= *loads_removed_counter, "The number of loads removed should not be more than all loads" ); } From ebfdc6d108334af1d7e1a55a4976a584aed35415 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 17:21:13 +0000 Subject: [PATCH 24/58] switch back to always reinserting last_load --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 993407f6bd6..b3bc90e4836 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -141,8 +141,8 @@ struct PerFuncLastLoadContext { } impl PerFuncLastLoadContext { - fn new(load_instruction: InstructionId, block_id: BasicBlockId) -> Self { - Self { num_loads: 0, load_instruction, block_id } + fn new(load_instruction: InstructionId, block_id: BasicBlockId, num_loads: u32) -> Self { + Self { num_loads, load_instruction, block_id } } } @@ -308,11 +308,12 @@ impl<'f> PerFunctionContext<'f> { self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); - let last_load = self - .last_loads - .entry(address) - .or_insert(PerFuncLastLoadContext::new(instruction, block_id)); - last_load.num_loads += 1; + let num_loads = + self.last_loads.get(&address).map_or(1, |context| context.num_loads + 1); + self.last_loads.insert( + address, + PerFuncLastLoadContext::new(instruction, block_id, num_loads), + ); } } Instruction::Store { address, value } => { From 4bbfc466ea16b34e2a9aa8f80800b04ac25c2da7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 17:22:36 +0000 Subject: [PATCH 25/58] fmt --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index b3bc90e4836..1d4cacd369b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -310,10 +310,8 @@ impl<'f> PerFunctionContext<'f> { let num_loads = self.last_loads.get(&address).map_or(1, |context| context.num_loads + 1); - self.last_loads.insert( - address, - PerFuncLastLoadContext::new(instruction, block_id, num_loads), - ); + let last_load = PerFuncLastLoadContext::new(instruction, block_id, num_loads); + self.last_loads.insert(address, last_load); } } Instruction::Store { address, value } => { From 76134535f70d30f0a134b3e5c79bfdaa2df198a1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 9 Sep 2024 16:46:27 +0000 Subject: [PATCH 26/58] recursively check load results, additional test for load results used in array constants --- compiler/noirc_driver/src/lib.rs | 1 + .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 298 +++++++++++++++++- .../references_aliasing/src/main.nr | 20 ++ tooling/nargo/src/ops/compile.rs | 2 +- 4 files changed, 304 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 31c279bc0f3..680d629c72f 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -419,6 +419,7 @@ fn compile_contract_inner( let mut functions = Vec::new(); let mut errors = Vec::new(); let mut warnings = Vec::new(); + for contract_function in &contract.functions { let function_id = contract_function.function_id; let is_entry_point = contract_function.is_entry_point; diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index dbebb9e6397..34b717f9422 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -265,6 +265,24 @@ impl<'f> PerFunctionContext<'f> { } } + fn count_increase_instruction( + &mut self, + value: ValueId, + instruction: InstructionId, + block_id: BasicBlockId, + ) { + if let Some(context) = self.load_results.get_mut(&value) { + context.uses += 1; + context.instructions_using_result.push((instruction, block_id)); + } + let array_const = self.inserter.function.dfg.get_array_constant(value); + if let Some((values, _)) = array_const { + for array_value in values { + self.count_increase_instruction(array_value, instruction, block_id); + } + } + } + fn analyze_instruction( &mut self, block_id: BasicBlockId, @@ -280,14 +298,6 @@ impl<'f> PerFunctionContext<'f> { return; } - // Track whether any load results were used in the instruction - self.inserter.function.dfg[instruction].for_each_value(|value| { - if let Some(context) = self.load_results.get_mut(&value) { - context.uses += 1; - context.instructions_using_result.push((instruction, block_id)); - } - }); - match &self.inserter.function.dfg[instruction] { Instruction::Load { address } => { let address = self.inserter.function.dfg.resolve(*address); @@ -410,6 +420,16 @@ impl<'f> PerFunctionContext<'f> { _ => (), } + let mut collect_values = Vec::new(); + // Track whether any load results were used in the instruction + self.inserter.function.dfg[instruction].for_each_value(|value| { + collect_values.push(value); + }); + + for value in collect_values { + self.count_increase_instruction(value, instruction, block_id); + } + self.track_rc_reload_state(instruction); } @@ -483,17 +503,23 @@ impl<'f> PerFunctionContext<'f> { self.inserter.function.dfg.data_bus = databus.map_values(|t| self.inserter.resolve(t)); } + fn recursive_count_increase(&mut self, value: ValueId) { + if let Some(context) = self.load_results.get_mut(&value) { + context.uses += 1; + } + let array_const = self.inserter.function.dfg.get_array_constant(value); + if let Some((values, _)) = array_const { + for array_value in values { + self.recursive_count_increase(array_value); + } + } + } + fn handle_terminator(&mut self, block: BasicBlockId, references: &mut Block) { self.inserter.map_terminator_in_place(block); let terminator = self.inserter.function.dfg[block].unwrap_terminator(); - terminator.for_each_value(|value| { - if let Some(context) = self.load_results.get_mut(&value) { - context.uses += 1; - } - }); - match terminator { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do TerminatorInstruction::Jmp { destination, arguments, .. } => { @@ -521,6 +547,15 @@ impl<'f> PerFunctionContext<'f> { self.mark_all_unknown(return_values, references); } } + + let mut collect_values = Vec::new(); + terminator.for_each_value(|value| { + collect_values.push(value); + }); + + for value in collect_values.iter() { + self.recursive_count_increase(*value); + } } /// The mem2reg pass is sometimes unable to determine certain known values @@ -546,7 +581,6 @@ impl<'f> PerFunctionContext<'f> { else { unreachable!("Should only have a load instruction here"); }; - // If the load result's counter is equal to zero we can safely remove that load instruction. if *uses == 0 { removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); @@ -604,7 +638,16 @@ impl<'f> PerFunctionContext<'f> { let is_not_used_in_reference_param = self.calls_reference_input.get(store_address).is_none(); - if remove_load && !is_reference_param && is_not_used_in_reference_param { + let is_reference_alias = block + .expressions + .get(store_address) + .map_or(false, |expression| matches!(expression, Expression::Dereference(_))); + + if remove_load + && !is_reference_param + && is_not_used_in_reference_param + && !is_reference_alias + { self.instructions_to_remove.insert(*store_instruction); if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; @@ -1156,4 +1199,227 @@ mod tests { // Should only have one instruction in b3 assert_eq!(main.dfg[b3].instructions().len(), 1); } + + #[test] + fn keep_store_to_alias_in_loop_block() { + // This test makes sure the instruction `store Field 2 at v5` in b2 remains after mem2reg. + // Although the only instruction on v5 is a lone store without any loads, + // v5 is an alias of the reference v0 which is stored in v2. + // This test makes sure that we are not inadvertently removing stores to aliases across blocks. + // + // acir(inline) fn main f0 { + // b0(): + // v0 = allocate + // store Field 0 at v0 + // v2 = allocate + // store v0 at v2 + // jmp b1(Field 0) + // b1(v3: Field): + // v4 = eq v3, Field 0 + // jmpif v4 then: b2, else: b3 + // b2(): + // v5 = load v2 + // store Field 2 at v5 + // v8 = add v3, Field 1 + // jmp b1(v8) + // b3(): + // v9 = load v0 + // v10 = eq v9, Field 2 + // constrain v9 == Field 2 + // v11 = load v2 + // v12 = load v10 + // v13 = eq v12, Field 2 + // constrain v11 == Field 2 + // return + // } + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + + let v0 = builder.insert_allocate(Type::field()); + let zero = builder.numeric_constant(0u128, Type::field()); + builder.insert_store(v0, zero); + + let v2 = builder.insert_allocate(Type::field()); + // Construct alias + builder.insert_store(v2, v0); + let v2_type = builder.current_function.dfg.type_of_value(v2); + assert!(builder.current_function.dfg.value_is_reference(v2)); + + let b1 = builder.insert_block(); + builder.terminate_with_jmp(b1, vec![zero]); + + // Loop header + builder.switch_to_block(b1); + let v3 = builder.add_block_parameter(b1, Type::field()); + let is_zero = builder.insert_binary(v3, BinaryOp::Eq, zero); + + let b2 = builder.insert_block(); + let b3 = builder.insert_block(); + builder.terminate_with_jmpif(is_zero, b2, b3); + + // Loop body + builder.switch_to_block(b2); + let v5 = builder.insert_load(v2, v2_type.clone()); + let two = builder.numeric_constant(2u128, Type::field()); + builder.insert_store(v5, two); + let one = builder.numeric_constant(1u128, Type::field()); + let v3_plus_one = builder.insert_binary(v3, BinaryOp::Add, one); + builder.terminate_with_jmp(b1, vec![v3_plus_one]); + + builder.switch_to_block(b3); + let v9 = builder.insert_load(v0, Type::field()); + let _ = builder.insert_binary(v9, BinaryOp::Eq, two); + + builder.insert_constrain(v9, two, None); + let v11 = builder.insert_load(v2, v2_type); + let v12 = builder.insert_load(v11, Type::field()); + let _ = builder.insert_binary(v12, BinaryOp::Eq, two); + + builder.insert_constrain(v11, two, None); + builder.terminate_with_return(vec![]); + + let ssa = builder.finish(); + + // We expect the same result as above. + let ssa = ssa.mem2reg(); + + let main = ssa.main(); + assert_eq!(main.reachable_blocks().len(), 4); + + // The store from the original SSA should remain + assert_eq!(count_stores(main.entry_block(), &main.dfg), 2); + assert_eq!(count_stores(b2, &main.dfg), 1); + + assert_eq!(count_loads(b2, &main.dfg), 1); + assert_eq!(count_loads(b3, &main.dfg), 3); + } + + #[test] + fn accurate_tracking_of_load_results() { + // acir(inline) fn main f0 { + // b0(): + // v0 = allocate + // store Field 5 at v0 + // v2 = allocate + // store u32 10 at v2 + // v4 = load v0 + // v5 = load v2 + // v6 = allocate + // store v4 at v6 + // v7 = allocate + // store v5 at v7 + // v8 = load v6 + // v9 = load v7 + // v10 = load v6 + // v11 = load v7 + // v12 = allocate + // store Field 0 at v12 + // v15 = eq v11, u32 0 + // jmpif v15 then: b1, else: b2 + // b1(): + // v16 = load v12 + // v17 = add v16, v8 + // store v17 at v12 + // jmp b2() + // b2(): + // v18 = load v12 + // return [v18] + // } + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + + let v0 = builder.insert_allocate(Type::field()); + let five = builder.numeric_constant(5u128, Type::field()); + builder.insert_store(v0, five); + + let v2 = builder.insert_allocate(Type::field()); + let ten = builder.numeric_constant(10u128, Type::unsigned(32)); + builder.insert_store(v2, ten); + + let v4 = builder.insert_load(v0, Type::field()); + let v5 = builder.insert_load(v2, Type::unsigned(32)); + let v4_type = builder.current_function.dfg.type_of_value(v4); + let v5_type = builder.current_function.dfg.type_of_value(v5); + + let v6 = builder.insert_allocate(Type::field()); + builder.insert_store(v6, v4); + let v7 = builder.insert_allocate(Type::unsigned(32)); + builder.insert_store(v7, v5); + + let v8 = builder.insert_load(v6, v4_type.clone()); + let _v9 = builder.insert_load(v7, v5_type.clone()); + + let _v10 = builder.insert_load(v6, v4_type); + let v11 = builder.insert_load(v7, v5_type); + + let v12 = builder.insert_allocate(Type::field()); + let zero = builder.numeric_constant(0u128, Type::field()); + builder.insert_store(v12, zero); + + let zero_u32 = builder.numeric_constant(0u128, Type::unsigned(32)); + let v15 = builder.insert_binary(v11, BinaryOp::Eq, zero_u32); + + let b1 = builder.insert_block(); + let b2 = builder.insert_block(); + builder.terminate_with_jmpif(v15, b1, b2); + + builder.switch_to_block(b1); + + let v16 = builder.insert_load(v12, Type::field()); + let v17 = builder.insert_binary(v16, BinaryOp::Add, v8); + builder.insert_store(v12, v17); + + builder.terminate_with_jmp(b2, vec![]); + + builder.switch_to_block(b2); + let v18 = builder.insert_load(v12, Type::field()); + + // Include the load result as part of an array constant to check that we are accounting for arrays + // when updating the reference counts of load results. + // + // If we were not accounting for arrays appropriately, the load of v18 would be removed. + // If v18 is the last load of a reference and is inadvertently removed, + // any stores to v12 will then be potentially removed as well and the program will be broken. + let return_array = + builder.array_constant(vector![v18], Type::Array(Arc::new(vec![Type::field()]), 1)); + builder.terminate_with_return(vec![return_array]); + + let ssa = builder.finish(); + println!("{}", ssa); + + // Expected result: + // acir(inline) fn main f0 { + // b0(): + // v20 = allocate + // v21 = allocate + // v24 = allocate + // v25 = allocate + // v30 = allocate + // store Field 0 at v30 + // jmpif u1 0 then: b1, else: b2 + // b1(): + // store Field 5 at v30 + // jmp b2() + // b2(): + // v33 = load v30 + // return [v33] + // } + let ssa = ssa.mem2reg(); + println!("{}", ssa); + + let main = ssa.main(); + assert_eq!(main.reachable_blocks().len(), 3); + + // A single store from the entry block should remain. + // If we are not appropriately handling unused stores across a function, + // we would expect all five stores from the original SSA to remain. + assert_eq!(count_stores(main.entry_block(), &main.dfg), 1); + // The store from the conditional block should remain, + // as it is loaded from in a successor block and used in the return terminator. + assert_eq!(count_stores(b1, &main.dfg), 1); + + assert_eq!(count_loads(main.entry_block(), &main.dfg), 0); + assert_eq!(count_loads(b1, &main.dfg), 0); + assert_eq!(count_loads(b2, &main.dfg), 1); + } } diff --git a/test_programs/compile_success_empty/references_aliasing/src/main.nr b/test_programs/compile_success_empty/references_aliasing/src/main.nr index 0d96bc2a734..1c62e051e9e 100644 --- a/test_programs/compile_success_empty/references_aliasing/src/main.nr +++ b/test_programs/compile_success_empty/references_aliasing/src/main.nr @@ -5,6 +5,8 @@ fn main() { assert(*xref == 101); regression_2445(); + + assert(5 == struct_field_refs_across_blocks(MyStruct { a: 5, b: 10 })[0]); } fn increment(mut r: &mut Field) { @@ -26,3 +28,21 @@ fn regression_2445() { assert(**array[0] == 2); assert(**array[1] == 2); } + +struct MyStruct { + a: Field, + b: u32, +} + +fn struct_field_refs_across_blocks(mut my_struct: MyStruct) -> [Field; 1] { + [compute_dummy_hash(my_struct.a, my_struct.b, 20)] +} + +fn compute_dummy_hash(input: Field, rhs: u32, in_len: u32) -> Field { + let mut res = 0; + if rhs < in_len { + res += input; + } + res +} + diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index cd9ccf67957..81401f66f05 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -36,7 +36,7 @@ pub fn compile_workspace( }) .collect(); let contract_results: Vec> = contract_packages - .par_iter() + .iter() .map(|package| compile_contract(file_manager, parsed_files, package, compile_options)) .collect(); From cec0c456bd0999e8c48683e172c4694d727f0d7d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 9 Sep 2024 17:04:57 +0000 Subject: [PATCH 27/58] fix const in test --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index ad4841316a2..b127938a9ec 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -1333,7 +1333,7 @@ mod tests { builder.insert_store(v0, five); let v2 = builder.insert_allocate(Type::unsigned(32)); - let ten = builder.numeric_constant(5u128, Type::unsigned(32)); + let ten = builder.numeric_constant(10u128, Type::unsigned(32)); builder.insert_store(v2, ten); let v4 = builder.insert_load(v0, Type::field()); From 9506f8ba799bc692163081f5458d33c698ebade7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 9 Sep 2024 17:09:14 +0000 Subject: [PATCH 28/58] nargo fmt --- .../compile_success_empty/references_aliasing/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/compile_success_empty/references_aliasing/src/main.nr b/test_programs/compile_success_empty/references_aliasing/src/main.nr index 76425176415..b2b625477e7 100644 --- a/test_programs/compile_success_empty/references_aliasing/src/main.nr +++ b/test_programs/compile_success_empty/references_aliasing/src/main.nr @@ -56,4 +56,4 @@ fn compute_dummy_hash(input: Field, rhs: u32, in_len: u32) -> Field { res += input; } res -} \ No newline at end of file +} From df03d7d0ddb0db52bf51f865c243353f1151316a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 12 Sep 2024 19:37:09 +0000 Subject: [PATCH 29/58] dirty mem2reg changes to get things passing w/ aztec --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 522 ++++++++++++++++-- 1 file changed, 474 insertions(+), 48 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index b127938a9ec..890c307be6d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -96,6 +96,33 @@ impl Ssa { context.remove_instructions(); context.update_data_bus(); } + // for (id, function) in self.functions.iter() { + // if function.name() != "create_note" { + // continue; + // } + // let mut block_order = PostOrder::with_function(function).into_vec(); + // block_order.reverse(); + // let mut allocate_values = HashSet::default(); + // for block in block_order { + // // let references = self.find_starting_references(block); + // // self.analyze_block(block, references); + // let instructions = function.dfg[block].instructions(); + // for instruction in instructions { + // let results = function.dfg.instruction_results(*instruction); + // match &function.dfg[*instruction] { + // Instruction::Allocate => { + // allocate_values.insert(results[0]); + // } + // Instruction::Store { address, value } => { + // allocate_values.remove(address); + // } + // _ => {} + // } + // } + // } + // dbg!(allocate_values.clone()); + // } + self } } @@ -128,6 +155,13 @@ struct PerFunctionContext<'f> { /// Track whether the last instruction is an inc_rc/dec_rc instruction. /// If it is we should not remove any known store values. inside_rc_reload: bool, + + // The map of block ids represents the blocks where a store was used + // And the usage per block + store_count: HashMap)>, + + // The index of the last load instruction in a given block + load_instruction_locations: HashMap<(ValueId, BasicBlockId), HashMap>, } #[derive(Debug, Clone)] @@ -152,13 +186,17 @@ struct PerFuncLoadResultContext { uses: u32, /// Load instruction that produced a given load result load_instruction: InstructionId, + /// Block of the load instruction that produced a given result + block_id: BasicBlockId, /// Instructions that use a given load result instructions_using_result: Vec<(InstructionId, BasicBlockId)>, + /// Terminators that use a given load result + terminators_using_result: Vec<(TerminatorInstruction, BasicBlockId)>, } impl PerFuncLoadResultContext { - fn new(load_instruction: InstructionId) -> Self { - Self { uses: 0, load_instruction, instructions_using_result: vec![] } + fn new(load_instruction: InstructionId, block_id: BasicBlockId) -> Self { + Self { uses: 0, load_instruction, block_id, instructions_using_result: vec![], terminators_using_result: vec![], } } } @@ -177,6 +215,8 @@ impl<'f> PerFunctionContext<'f> { load_results: HashMap::default(), calls_reference_input: HashSet::default(), inside_rc_reload: false, + store_count: HashMap::default(), + load_instruction_locations: HashMap::default(), } } @@ -236,7 +276,7 @@ impl<'f> PerFunctionContext<'f> { // 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.remove_stores_that_do_not_alias_parameters(&references, block); } self.blocks.insert(block, references); @@ -244,7 +284,7 @@ impl<'f> PerFunctionContext<'f> { /// 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) { + fn remove_stores_that_do_not_alias_parameters(&mut self, references: &Block, block_id: BasicBlockId) { let parameters = self.inserter.function.parameters().iter(); let reference_parameters = parameters .filter(|param| self.inserter.function.dfg.value_is_reference(**param)) @@ -259,6 +299,17 @@ impl<'f> PerFunctionContext<'f> { // If `allocation_aliases_parameter` is known to be false if allocation_aliases_parameter == Some(false) { self.instructions_to_remove.insert(*instruction); + if let Some(context) = self.load_results.get_mut(allocation) { + context.uses -= 1; + } + if let Some((store_uses, blocks)) = self.store_count.get_mut(allocation) { + if *store_uses != 0 { + *store_uses -= 1; + } + // if let Some(block_uses) = blocks.get_mut(&block_id) { + // *block_uses -= 1; + // } + } } } } @@ -271,10 +322,33 @@ impl<'f> PerFunctionContext<'f> { instruction: InstructionId, block_id: BasicBlockId, ) { + // if value.to_usize() == 31929 { + // dbg!(self.inserter.function.dfg[instruction].clone()); + // } if let Some(context) = self.load_results.get_mut(&value) { context.uses += 1; context.instructions_using_result.push((instruction, block_id)); } + if let Some((store_uses, blocks)) = self.store_count.get_mut(&value) { + // TODO: add check we are not in an allocation + match self.inserter.function.dfg[instruction] { + // We should never expect an allocate but we check it here anyway + Instruction::Allocate => {}, + // We are post-processing reference counts after an instruction so we should not increase + // on the store itself + // Instruction::Store { .. } => {}, + _ => { + *store_uses += 1; + // blocks.insert(block_id); + if let Some(block_uses) = blocks.get_mut(&block_id) { + *block_uses += 1; + } else { + blocks.insert(block_id, 1); + } + } + } + // *store_uses += 1; + } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { @@ -298,6 +372,16 @@ impl<'f> PerFunctionContext<'f> { return; } + let mut collect_values = Vec::new(); + // Track whether any load results were used in the instruction + self.inserter.function.dfg[instruction].for_each_value(|value| { + collect_values.push(value); + }); + + for value in collect_values { + self.count_increase_instruction(value, instruction, block_id); + } + match &self.inserter.function.dfg[instruction] { Instruction::Load { address } => { let address = self.inserter.function.dfg.resolve(*address); @@ -309,6 +393,17 @@ impl<'f> PerFunctionContext<'f> { if let Some(value) = references.get_known_value(address) { self.inserter.map_value(result, value); self.instructions_to_remove.insert(instruction); + if let Some(context) = self.load_results.get_mut(&address) { + context.uses -= 1; + } + if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { + if *store_uses != 0 { + *store_uses -= 1; + } + // if let Some(block_uses) = blocks.get_mut(&block_id) { + // *block_uses -= 1; + // } + } } else { references.mark_value_used(address, self.inserter.function); @@ -327,12 +422,31 @@ impl<'f> PerFunctionContext<'f> { // Mark that we know a load result is equivalent to the address of a load. references.set_known_value(result, address); - self.load_results.insert(result, PerFuncLoadResultContext::new(instruction)); + self.load_results.insert(result, PerFuncLoadResultContext::new(instruction, block_id)); let num_loads = self.last_loads.get(&address).map_or(1, |context| context.num_loads + 1); let last_load = PerFuncLastLoadContext::new(instruction, block_id, num_loads); self.last_loads.insert(address, last_load); + + let terminator = self.inserter.function.dfg[block_id].unwrap_terminator(); + let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); + if is_return { + let instruction_index = self.inserter.function.dfg[block_id].instructions().len(); + // dbg!(block_id); + // dbg!(instruction_index); + // if block_id.to_usize() == 159 { + // println!("address {}", address); + // } + if let Some(result_to_index_map) = self.load_instruction_locations.get_mut(&(address, block_id)) { + result_to_index_map.insert(result, instruction_index); + } else { + let mut new_map = HashMap::default(); + new_map.insert(result, instruction_index); + self.load_instruction_locations.insert((address, block_id), new_map); + } + } + } } Instruction::Store { address, value } => { @@ -348,6 +462,15 @@ impl<'f> PerFunctionContext<'f> { if let Some(context) = self.load_results.get_mut(&value) { context.uses -= 1; } + if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { + // *store_uses -= 1; + if *store_uses != 0 { + *store_uses -= 1; + } + // if let Some(block_uses) = blocks.get_mut(&block_id) { + // *block_uses -= 1; + // } + } } let known_value = references.get_known_value(value); @@ -355,11 +478,46 @@ impl<'f> PerFunctionContext<'f> { let known_value_is_address = known_value == address; if known_value_is_address && !self.inside_rc_reload { self.instructions_to_remove.insert(instruction); + if let Some(context) = self.load_results.get_mut(&address) { + context.uses -= 1; + } + if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { + // *store_uses -= 1; + if *store_uses != 0 { + *store_uses -= 1; + } + // if let Some(block_uses) = blocks.get_mut(&block_id) { + // *block_uses -= 1; + // } + } + } + else { + // references.last_stores.insert(address, instruction); + + // let num_stores = self.store_count.get(&address).copied().unwrap_or(1); + // self.store_count.insert(address, num_stores); } + } + else { + // references.last_stores.insert(address, instruction); + + // let num_stores = self.store_count.get(&address).copied().unwrap_or(1); + // self.store_count.insert(address, num_stores); } + references.set_known_value(address, value); + // let instruction_index = self.inserter.function.dfg[block_id].instructions().len(); + // dbg!(block_id); + // dbg!(instruction_index); references.last_stores.insert(address, instruction); + + let (num_stores, blocks) = self.store_count.get(&address).cloned().unwrap_or((1, HashMap::default())); + // let mut blocks = HashSet::default(); + // if *num_stores == 1 { + // blocks.insert(block_id); + // } + self.store_count.insert(address, (num_stores, blocks)); } Instruction::Allocate => { // Register the new reference @@ -412,7 +570,14 @@ impl<'f> PerFunctionContext<'f> { Instruction::Call { arguments, .. } => { for arg in arguments { if self.inserter.function.dfg.value_is_reference(*arg) { - self.calls_reference_input.insert(*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.calls_reference_input.insert(*arg); } } self.mark_all_unknown(arguments, references); @@ -420,15 +585,15 @@ impl<'f> PerFunctionContext<'f> { _ => (), } - let mut collect_values = Vec::new(); - // Track whether any load results were used in the instruction - self.inserter.function.dfg[instruction].for_each_value(|value| { - collect_values.push(value); - }); + // let mut collect_values = Vec::new(); + // // Track whether any load results were used in the instruction + // self.inserter.function.dfg[instruction].for_each_value(|value| { + // collect_values.push(value); + // }); - for value in collect_values { - self.count_increase_instruction(value, instruction, block_id); - } + // for value in collect_values { + // self.count_increase_instruction(value, instruction, block_id); + // } self.track_rc_reload_state(instruction); } @@ -503,14 +668,24 @@ impl<'f> PerFunctionContext<'f> { self.inserter.function.dfg.data_bus = databus.map_values(|t| self.inserter.resolve(t)); } - fn recursive_count_increase(&mut self, value: ValueId) { + fn recursive_count_increase(&mut self, value: ValueId, terminator: &TerminatorInstruction, block_id: BasicBlockId) { if let Some(context) = self.load_results.get_mut(&value) { context.uses += 1; + context.terminators_using_result.push((terminator.clone(), block_id)); + } + if let Some((store_uses, blocks)) = self.store_count.get_mut(&value) { + *store_uses += 1; + // blocks.insert(block_id); + if let Some(block_uses) = blocks.get_mut(&block_id) { + *block_uses += 1; + } else { + blocks.insert(block_id, 1); + } } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { - self.recursive_count_increase(array_value); + self.recursive_count_increase(array_value, terminator, block_id); } } } @@ -518,7 +693,7 @@ impl<'f> PerFunctionContext<'f> { fn handle_terminator(&mut self, block: BasicBlockId, references: &mut Block) { self.inserter.map_terminator_in_place(block); - let terminator = self.inserter.function.dfg[block].unwrap_terminator(); + let terminator: &TerminatorInstruction = self.inserter.function.dfg[block].unwrap_terminator(); match terminator { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do @@ -551,10 +726,11 @@ impl<'f> PerFunctionContext<'f> { let mut collect_values = Vec::new(); terminator.for_each_value(|value| { collect_values.push(value); - }); + }); + let terminator = terminator.clone(); for value in collect_values.iter() { - self.recursive_count_increase(*value); + self.recursive_count_increase(*value, &terminator, block); } } @@ -568,21 +744,58 @@ impl<'f> PerFunctionContext<'f> { let removed_loads = self.remove_unused_loads(); let remaining_last_stores = self.remove_unloaded_last_stores(&removed_loads); self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); + + // After possibly removing some instructions we need to map all the instructions + // TODO: check whether we removed any instructions, if we have not skip this check + let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); + block_order.reverse(); + for block in block_order { + let instructions = self.inserter.function.dfg[block].take_instructions(); + for instruction in instructions { + if !self.instructions_to_remove.contains(&instruction) { + self.inserter.push_instruction(instruction, block); + } + } + self.inserter.map_terminator_in_place(block); + } } /// Cleanup remaining loads across the entire function /// Remove any loads whose reference counter is zero. /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { + let mut all_block_params: HashSet = HashSet::default(); + for (block_id, _) in self.blocks.iter() { + let block_params = self.inserter.function.dfg.block_parameters(*block_id); + all_block_params.extend(block_params.iter()); + } + let mut removed_loads = HashMap::default(); - for (_, PerFuncLoadResultContext { uses, load_instruction, .. }) in self.load_results.iter() + for (_, PerFuncLoadResultContext { uses, load_instruction, block_id, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { unreachable!("Should only have a load instruction here"); }; + + // let is_reference_result = self.inserter.function.dfg.value_is_reference(*result); + // If the load result's counter is equal to zero we can safely remove that load instruction. if *uses == 0 { + + // if all_block_params.contains(&address) { + // continue; + // } + if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { + // println!("have store use"); + if *store_uses != 0 { + *store_uses -= 1; + } + // if let Some(block_uses) = blocks.get_mut(block_id) { + // *block_uses -= 1; + // } + } + removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); self.instructions_to_remove.insert(*load_instruction); @@ -591,6 +804,16 @@ impl<'f> PerFunctionContext<'f> { removed_loads } + fn recursively_check_address_in_terminator(&self, return_value: ValueId, store_address: ValueId, is_return_value: &mut bool) { + *is_return_value = return_value == store_address || *is_return_value; + let array_const = self.inserter.function.dfg.get_array_constant(return_value); + if let Some((values, _)) = array_const { + for array_value in values { + self.recursively_check_address_in_terminator(array_value, store_address, is_return_value); + } + } + } + /// Cleanup remaining stores across the entire function. /// 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. @@ -599,13 +822,29 @@ impl<'f> PerFunctionContext<'f> { &mut self, removed_loads: &HashMap, ) -> HashMap { + let mut all_block_params: HashSet = HashSet::default(); + for (block_id, _) in self.blocks.iter() { + let block_params = self.inserter.function.dfg.block_parameters(*block_id); + all_block_params.extend(block_params.iter()); + } + // let func_params = self.inserter.function.parameters(); + let mut remaining_last_stores: HashMap = HashMap::default(); for (block_id, block) in self.blocks.iter() { + self.inserter.map_terminator_in_place(*block_id); + let block_params = self.inserter.function.dfg.block_parameters(*block_id); for (store_address, store_instruction) in block.last_stores.iter() { + if self.instructions_to_remove.contains(store_instruction) { + continue; + } let is_reference_param = block_params.contains(store_address); - let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); + let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); + let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] + else { + unreachable!("Should only have a store instruction"); + }; let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); // Determine whether any loads that reference this store address // have been removed while cleaning up unused loads. @@ -616,6 +855,14 @@ impl<'f> PerFunctionContext<'f> { is_return_value = return_value == *store_address || is_return_value; }); + let mut new_is_return_value = false; + terminator.for_each_value(|return_value| { + self.recursively_check_address_in_terminator(return_value, *store_address, &mut new_is_return_value); + }); + if new_is_return_value != is_return_value { + println!("{}", store_address); + } + // If the last load of a store is not part of the block with a return terminator, // we can safely remove this store. let last_load_not_in_return = self @@ -623,38 +870,186 @@ impl<'f> PerFunctionContext<'f> { .get(store_address) .map(|context| context.block_id != *block_id) .unwrap_or(true); - !is_return_value && last_load_not_in_return - } else if let (Some(context), Some(loads_removed_counter)) = - (self.last_loads.get(store_address), removed_loads.get(store_address)) + // println!("remove store {} in return: {}", store_address, !is_return_value && last_load_not_in_return); + // if let Some((store_uses, blocks)) = self.store_count.get(store_address) { + // dbg!(block_id); + // if let Some(return_block_uses) = blocks.get(&block_id) { + // println!("store uses contains return block {} with {} uses in the return block", block_id, return_block_uses); + // !is_return_value && last_load_not_in_return && *return_block_uses <= 1 + // } else { + // println!("{} with {} uses, !is_return_value && last_load_not_in_return: {}", store_address, store_uses, !is_return_value && last_load_not_in_return); + // !is_return_value && last_load_not_in_return && *store_uses <= 1 + // } + // // println!("{} with {} uses, !is_return_value && last_load_not_in_return: {}", store_address, store_uses, !is_return_value && last_load_not_in_return); + // // dbg!(blocks.clone()); + // // !is_return_value && last_load_not_in_return && *store_uses <= 1 + // // !is_return_value && last_load_not_in_return && *store_uses == 0 || store_uses - 1 == 0 + // // !is_return_value && last_load_not_in_return && *store_uses > 1 + // } else { + // !is_return_value && last_load_not_in_return + // } + // dbg!(self.load_instruction_locations.clone()); + + + let mut max_load_index: usize = 0; + if let Some(x) = self.load_instruction_locations.get(&(*store_address, *block_id)) { + // x.value + // if store_address.to_usize() == 8823 { + // dbg!(x.values()); + // } + for index in x.values() { + if *index > max_load_index { + max_load_index = *index; + } + } + } + let store_index = self.inserter.function.dfg[*block_id].instructions().iter().position(|id| *id == *store_instruction).unwrap(); + // if store_index > max_load_index { + // dbg!(store_index); + // dbg!(max_load_index); + // let (uses, blocks) = self.store_count.get(store_address).unwrap(); + // let uses_in_block = blocks.get(block_id).unwrap(); + // println!("return_block {} store_address {} store_index {} max_load_index {} uses_in_block {} uses {}", block_id, store_address, store_index, max_load_index, uses_in_block, uses); + // } + // let is_value_param = all_block_params.contains(&value); + + // if store_index == 1 && max_load_index == 0 && block_id.to_usize() == 159 { + // dbg!(store_address); + // dbg!(value); + // dbg!(is_value_param); + // dbg!(is_reference_param); + // dbg!(all_block_params.contains(store_address)); + // dbg!(self.load_instruction_locations.get(&(*store_address, *block_id)).clone()); + // } + // let (uses, blocks) = self.store_count.get(store_address).unwrap(); + // let uses_in_block = blocks.get(block_id).unwrap(); + // !is_return_value && last_load_not_in_return && max_load_index == 0 + // TODO: we should not need this uses in block, as we check whether the reference is used in the terminator + // If the max_load_index is zero we may have constant folding out a load?? + // !is_return_value && last_load_not_in_return && store_index > max_load_index && max_load_index != 0 + !is_return_value && store_index > max_load_index && max_load_index != 0 + + // false + } else if let (Some(context), Some(loads_removed_counter), Some((store_uses, _))) = + (self.last_loads.get(store_address), removed_loads.get(store_address), self.store_count.get(store_address)) { // `last_loads` contains the total number of loads for a given load address // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. - context.num_loads == *loads_removed_counter + + // if context.num_loads == *loads_removed_counter { + // dbg!(loads_removed_counter); + // dbg!(store_uses); + // // dbg!(self.store_count.get(store_address)); + // dbg!(store_address); + // } + + (store_uses - 1) == 0 && context.num_loads == *loads_removed_counter + // context.num_loads == *loads_removed_counter && store_uses == loads_removed_counter + // context.num_loads == *loads_removed_counter && (store_uses - 1) == *loads_removed_counter + // if context.num_loads == *loads_removed_counter { + // } + + // false } else { + let last_load_exists = self.last_loads.get(store_address).is_some(); + if let Some((store_uses, _)) = self.store_count.get(store_address) { + // if last_load_exists { + // dbg!(self.store_count.get(store_address)); + // } + // if !last_load_exists && *store_uses == 0 { + // println!("!last_load_exists && *store_uses == 0 for {}", store_address); + // } + !last_load_exists && *store_uses <= 1 + } else { + // println!("!last_load_exists for {}", store_address); + !last_load_exists + } + // TODO: getting a failure here + // self.store_count.get(store_address) + // if !last_load_exists { + // println!("{} num uses: {}", store_address, self.store_count.get(store_address).unwrap()); + // } + // !last_load_exists + // false // Otherwise just check whether a load exists at all for this store address - self.last_loads.get(store_address).is_none() + // self.last_loads.get(store_address).is_none() }; - let is_not_used_in_reference_param = - self.calls_reference_input.get(store_address).is_none(); + if remove_load == true && store_address.to_usize() == 24100 { + println!("got here"); + } + + let is_used_in_reference_param = + self.calls_reference_input.get(store_address).is_some(); let is_reference_alias = block .expressions .get(store_address) .map_or(false, |expression| matches!(expression, Expression::Dereference(_))); + + // let is_value_reference = self.inserter.function.dfg.value_is_reference(value); + let is_value_param = all_block_params.contains(&value); + // let is_func_param = func_params.contains(&store_address); if remove_load && !is_reference_param - && is_not_used_in_reference_param - && !is_reference_alias - { + && !is_used_in_reference_param + && !is_reference_alias + // && !is_func_param + // && !is_value_reference + // && !is_value_param + { + // println!("{}", store_address); + // if is_value_param { + // println!("got value alias"); + // } + // let is_all_blocks_ref_param = all_block_params.contains(store_address); + // if is_all_blocks_ref_param { + // println!("removing store that is a block param"); + // } + + // if is_value_param { + // println!("{}", store_address); + // } + // let is_value_reference = self.inserter.function.dfg.value_is_reference(value); + // dbg!(is_value_reference); + // if is_value_reference { + // dbg!(store_address); + // dbg!(is_reference_alias); + // } + // let is_all_blocks_param = all_block_params.contains(store_address); + // if is_all_blocks_param != is_reference_param { + // dbg!(store_address); + // } self.instructions_to_remove.insert(*store_instruction); + if let Some((store_uses, blocks)) = self.store_count.get_mut(&store_address) { + if *store_uses != 0 { + *store_uses -= 1; + } + // if let Some(block_uses) = blocks.get_mut(&block_id) { + // *block_uses -= 1; + // } + } if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; } } else if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { + // if is_reference_alias || is_reference_param || is_used_in_reference_param { + // dbg!(store_address); + // } *counter += 1; } else { + // if is_reference_alias || is_reference_param || is_used_in_reference_param { + // dbg!(store_address); + // dbg!(is_reference_alias); + // dbg!(is_reference_param); + // dbg!(is_reference_alias); + // } + // TODO: temp make this 2, we want to account for remaining last stores better + // We do not count stores to reference params as a remaining store for possible removal + // if !is_reference_param { + // remaining_last_stores.insert(*store_address, (*store_instruction, 1)); + // } remaining_last_stores.insert(*store_address, (*store_instruction, 1)); } } @@ -668,6 +1063,12 @@ impl<'f> PerFunctionContext<'f> { removed_loads: &HashMap, remaining_last_stores: &HashMap, ) { + let mut all_block_params: HashSet = HashSet::default(); + for (block_id, _) in self.blocks.iter() { + let block_params = self.inserter.function.dfg.block_parameters(*block_id); + all_block_params.extend(block_params.iter()); + } + // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, uses, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] @@ -677,12 +1078,14 @@ impl<'f> PerFunctionContext<'f> { remaining_last_stores.contains_key(&address) && *uses > 0 }); + // let mut blocks_to_map_instructions = Vec::new(); + // let mut blocks_to_map_terminators = HashSet::default(); for (store_address, (store_instruction, store_counter)) in remaining_last_stores { let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] else { unreachable!("Should only have a store instruction"); }; - + if let (Some(context), Some(loads_removed_counter)) = (self.last_loads.get(store_address), removed_loads.get(store_address)) { @@ -692,10 +1095,25 @@ impl<'f> PerFunctionContext<'f> { ); } + // let is_reference_value = self.inserter.function.dfg.value_is_reference(value); + // if is_reference_value { + // dbg!(store_counter); + // dbg!(store_address); + // } + + // if all_block_params.contains(store_address) { + // continue; + // } + // We only want to remove last stores referencing a single address. if *store_counter != 0 { continue; } + // let is_used_in_reference_param = + // self.calls_reference_input.get(store_address).is_some(); + // if is_used_in_reference_param { + // dbg!(store_address); + // } self.instructions_to_remove.insert(*store_instruction); @@ -710,25 +1128,23 @@ impl<'f> PerFunctionContext<'f> { continue; } - self.inserter.map_value(*result, value); - for (instruction, block_id) in &context.instructions_using_result { - if self.instructions_to_remove.contains(instruction) { - continue; - } + // if address.to_usize() == 22868 { + // dbg!(context.clone()); + // } - let new_instruction = self.inserter.push_instruction(*instruction, *block_id); - let instructions = self.inserter.function.dfg[*block_id].instructions_mut(); + self.inserter.map_value(*result, value); - // Re-assign or delete any mapped instructions after the final loads were removed. - if let Some(index) = instructions.iter().position(|v| *v == *instruction) { - if let Some(new_instruction) = new_instruction { - instructions[index] = new_instruction; - } else { - instructions.remove(index); - } - } - } + // for (instruction, block_id) in &context.instructions_using_result { + // if self.instructions_to_remove.contains(instruction) { + // continue; + // } + // blocks_to_map_instructions.push(*block_id); + // } + + // for (_, block_id) in &context.terminators_using_result { + // blocks_to_map_terminators.insert(*block_id); + // } self.instructions_to_remove.insert(context.load_instruction); } @@ -1422,4 +1838,14 @@ mod tests { assert_eq!(count_loads(b1, &main.dfg), 0); assert_eq!(count_loads(b2, &main.dfg), 1); } + + #[test] + fn keep_last_store_with_value_block_param() { + + } + + // TODO: + // 1. Add a test that a removed load result is mapped throughout all the instructions, + // e.g. if the result of a new instruction caused by a removed load is used as a store value + // 2. Add a test that } From 9a6611a4749db48a95def208b760f16de637f920 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 16:32:55 +0000 Subject: [PATCH 30/58] handle all terminators and block params of a function + store uses hack --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 524 +++++------------- .../references_testing/Nargo.toml | 7 + .../references_testing/src/main.nr | 86 +++ 3 files changed, 229 insertions(+), 388 deletions(-) create mode 100644 test_programs/execution_success/references_testing/Nargo.toml create mode 100644 test_programs/execution_success/references_testing/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index ac1251c07df..a3bf380c496 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -96,32 +96,6 @@ impl Ssa { context.remove_instructions(); context.update_data_bus(); } - // for (id, function) in self.functions.iter() { - // if function.name() != "create_note" { - // continue; - // } - // let mut block_order = PostOrder::with_function(function).into_vec(); - // block_order.reverse(); - // let mut allocate_values = HashSet::default(); - // for block in block_order { - // // let references = self.find_starting_references(block); - // // self.analyze_block(block, references); - // let instructions = function.dfg[block].instructions(); - // for instruction in instructions { - // let results = function.dfg.instruction_results(*instruction); - // match &function.dfg[*instruction] { - // Instruction::Allocate => { - // allocate_values.insert(results[0]); - // } - // Instruction::Store { address, value } => { - // allocate_values.remove(address); - // } - // _ => {} - // } - // } - // } - // dbg!(allocate_values.clone()); - // } self } @@ -154,10 +128,12 @@ struct PerFunctionContext<'f> { // The map of block ids represents the blocks where a store was used // And the usage per block - store_count: HashMap)>, + // TODO: Get rid of this map. This is being used for debugging + // as we are not catching certain usages of a store that we should be + store_count: HashMap, // The index of the last load instruction in a given block - load_instruction_locations: HashMap<(ValueId, BasicBlockId), HashMap>, + return_block_load_locations: HashMap<(ValueId, BasicBlockId), usize>, } #[derive(Debug, Clone)] @@ -192,7 +168,13 @@ struct PerFuncLoadResultContext { impl PerFuncLoadResultContext { fn new(load_instruction: InstructionId, block_id: BasicBlockId) -> Self { - Self { uses: 0, load_instruction, block_id, instructions_using_result: vec![], terminators_using_result: vec![], } + Self { + uses: 0, + load_instruction, + block_id, + instructions_using_result: vec![], + terminators_using_result: vec![], + } } } @@ -211,7 +193,7 @@ impl<'f> PerFunctionContext<'f> { load_results: HashMap::default(), calls_reference_input: HashSet::default(), store_count: HashMap::default(), - load_instruction_locations: HashMap::default(), + return_block_load_locations: HashMap::default(), } } @@ -271,7 +253,7 @@ impl<'f> PerFunctionContext<'f> { // 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, block); + self.remove_stores_that_do_not_alias_parameters(&references); } self.blocks.insert(block, references); @@ -279,7 +261,7 @@ impl<'f> PerFunctionContext<'f> { /// 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, block_id: BasicBlockId) { + fn remove_stores_that_do_not_alias_parameters(&mut self, references: &Block) { let parameters = self.inserter.function.parameters().iter(); let reference_parameters = parameters .filter(|param| self.inserter.function.dfg.value_is_reference(**param)) @@ -297,13 +279,10 @@ impl<'f> PerFunctionContext<'f> { if let Some(context) = self.load_results.get_mut(allocation) { context.uses -= 1; } - if let Some((store_uses, blocks)) = self.store_count.get_mut(allocation) { + if let Some(store_uses) = self.store_count.get_mut(allocation) { if *store_uses != 0 { *store_uses -= 1; } - // if let Some(block_uses) = blocks.get_mut(&block_id) { - // *block_uses -= 1; - // } } } } @@ -317,33 +296,21 @@ impl<'f> PerFunctionContext<'f> { instruction: InstructionId, block_id: BasicBlockId, ) { - // if value.to_usize() == 31929 { - // dbg!(self.inserter.function.dfg[instruction].clone()); - // } if let Some(context) = self.load_results.get_mut(&value) { context.uses += 1; context.instructions_using_result.push((instruction, block_id)); } - if let Some((store_uses, blocks)) = self.store_count.get_mut(&value) { - // TODO: add check we are not in an allocation + if let Some(store_uses) = self.store_count.get_mut(&value) { match self.inserter.function.dfg[instruction] { // We should never expect an allocate but we check it here anyway - Instruction::Allocate => {}, - // We are post-processing reference counts after an instruction so we should not increase - // on the store itself - // Instruction::Store { .. } => {}, + Instruction::Allocate => {} + // We are pre-processing reference counts before analyzing an instruction so we should increase + // on any usage of a store address, including the store itself. If a store is removed, we will decrement its count. _ => { *store_uses += 1; - // blocks.insert(block_id); - if let Some(block_uses) = blocks.get_mut(&block_id) { - *block_uses += 1; - } else { - blocks.insert(block_id, 1); - } } } - // *store_uses += 1; - } + } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { @@ -391,13 +358,10 @@ impl<'f> PerFunctionContext<'f> { if let Some(context) = self.load_results.get_mut(&address) { context.uses -= 1; } - if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { + if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { *store_uses -= 1; } - // if let Some(block_uses) = blocks.get_mut(&block_id) { - // *block_uses -= 1; - // } } } else { references.mark_value_used(address, self.inserter.function); @@ -417,31 +381,23 @@ impl<'f> PerFunctionContext<'f> { // Mark that we know a load result is equivalent to the address of a load. references.set_known_value(result, address); - self.load_results.insert(result, PerFuncLoadResultContext::new(instruction, block_id)); + self.load_results + .insert(result, PerFuncLoadResultContext::new(instruction, block_id)); let num_loads = self.last_loads.get(&address).map_or(1, |context| context.num_loads + 1); let last_load = PerFuncLastLoadContext::new(instruction, block_id, num_loads); self.last_loads.insert(address, last_load); + // If we are in a return block we want to save the last location of a load let terminator = self.inserter.function.dfg[block_id].unwrap_terminator(); let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); if is_return { - let instruction_index = self.inserter.function.dfg[block_id].instructions().len(); - // dbg!(block_id); - // dbg!(instruction_index); - // if block_id.to_usize() == 159 { - // println!("address {}", address); - // } - if let Some(result_to_index_map) = self.load_instruction_locations.get_mut(&(address, block_id)) { - result_to_index_map.insert(result, instruction_index); - } else { - let mut new_map = HashMap::default(); - new_map.insert(result, instruction_index); - self.load_instruction_locations.insert((address, block_id), new_map); - } + let instruction_index = + self.inserter.function.dfg[block_id].instructions().len(); + self.return_block_load_locations + .insert((address, block_id), instruction_index); } - } } Instruction::Store { address, value } => { @@ -457,14 +413,10 @@ impl<'f> PerFunctionContext<'f> { if let Some(context) = self.load_results.get_mut(&value) { context.uses -= 1; } - if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { - // *store_uses -= 1; + if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { *store_uses -= 1; } - // if let Some(block_uses) = blocks.get_mut(&block_id) { - // *block_uses -= 1; - // } } } @@ -476,43 +428,19 @@ impl<'f> PerFunctionContext<'f> { if let Some(context) = self.load_results.get_mut(&address) { context.uses -= 1; } - if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { - // *store_uses -= 1; + if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { *store_uses -= 1; } - // if let Some(block_uses) = blocks.get_mut(&block_id) { - // *block_uses -= 1; - // } } - } - else { - // references.last_stores.insert(address, instruction); - - // let num_stores = self.store_count.get(&address).copied().unwrap_or(1); - // self.store_count.insert(address, num_stores); } - } - else { - // references.last_stores.insert(address, instruction); - - // let num_stores = self.store_count.get(&address).copied().unwrap_or(1); - // self.store_count.insert(address, num_stores); } - references.set_known_value(address, value); - // let instruction_index = self.inserter.function.dfg[block_id].instructions().len(); - // dbg!(block_id); - // dbg!(instruction_index); references.last_stores.insert(address, instruction); - let (num_stores, blocks) = self.store_count.get(&address).cloned().unwrap_or((1, HashMap::default())); - // let mut blocks = HashSet::default(); - // if *num_stores == 1 { - // blocks.insert(block_id); - // } - self.store_count.insert(address, (num_stores, blocks)); + let num_stores = self.store_count.get(&address).copied().unwrap_or(1); + self.store_count.insert(address, num_stores); } Instruction::Allocate => { // Register the new reference @@ -569,10 +497,9 @@ impl<'f> PerFunctionContext<'f> { if let Some(aliases) = references.aliases.get(expression) { aliases.for_each(|alias| { self.calls_reference_input.insert(alias); - }) + }); } } - // self.calls_reference_input.insert(*arg); } } self.mark_all_unknown(arguments, references); @@ -641,24 +568,23 @@ impl<'f> PerFunctionContext<'f> { self.inserter.function.dfg.data_bus = databus.map_values(|t| self.inserter.resolve(t)); } - fn recursive_count_increase(&mut self, value: ValueId, terminator: &TerminatorInstruction, block_id: BasicBlockId) { + fn terminator_count_increase( + &mut self, + value: ValueId, + terminator: &TerminatorInstruction, + block_id: BasicBlockId, + ) { if let Some(context) = self.load_results.get_mut(&value) { context.uses += 1; context.terminators_using_result.push((terminator.clone(), block_id)); } - if let Some((store_uses, blocks)) = self.store_count.get_mut(&value) { + if let Some(store_uses) = self.store_count.get_mut(&value) { *store_uses += 1; - // blocks.insert(block_id); - if let Some(block_uses) = blocks.get_mut(&block_id) { - *block_uses += 1; - } else { - blocks.insert(block_id, 1); - } } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { - self.recursive_count_increase(array_value, terminator, block_id); + self.terminator_count_increase(array_value, terminator, block_id); } } } @@ -666,9 +592,20 @@ impl<'f> PerFunctionContext<'f> { fn handle_terminator(&mut self, block: BasicBlockId, references: &mut Block) { self.inserter.map_terminator_in_place(block); - let terminator: &TerminatorInstruction = self.inserter.function.dfg[block].unwrap_terminator(); + let terminator: &TerminatorInstruction = + self.inserter.function.dfg[block].unwrap_terminator(); - match terminator { + let mut collect_values = Vec::new(); + terminator.for_each_value(|value| { + collect_values.push(value); + }); + + let terminator = terminator.clone(); + for value in collect_values.iter() { + self.terminator_count_increase(*value, &terminator, block); + } + + match &terminator { TerminatorInstruction::JmpIf { .. } => (), // Nothing to do TerminatorInstruction::Jmp { destination, arguments, .. } => { let destination_parameters = self.inserter.function.dfg[*destination].parameters(); @@ -695,15 +632,14 @@ impl<'f> PerFunctionContext<'f> { self.mark_all_unknown(return_values, references); } } + } - let mut collect_values = Vec::new(); - terminator.for_each_value(|value| { - collect_values.push(value); - }); - - let terminator = terminator.clone(); - for value in collect_values.iter() { - self.recursive_count_increase(*value, &terminator, block); + fn recursively_add_values(&self, value: ValueId, set: &mut HashSet) { + set.insert(value); + if let Some((elements, _)) = self.inserter.function.dfg.get_array_constant(value) { + for array_element in elements { + self.recursively_add_values(array_element, set); + } } } @@ -712,14 +648,22 @@ impl<'f> PerFunctionContext<'f> { /// We collect state about any final loads and stores to a given address during the initial mem2reg pass. /// We can then utilize this state to clean up any loads and stores that may have been missed. fn cleanup_function(&mut self) { + let mut all_terminator_values = HashSet::default(); + for (block_id, _) in self.blocks.iter() { + let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); + terminator.for_each_value(|value| { + self.recursively_add_values(value, &mut all_terminator_values); + }); + } + // Removing remaining unused loads during mem2reg can help expose removable stores that the initial // mem2reg pass deemed we could not remove due to the existence of those unused loads. let removed_loads = self.remove_unused_loads(); - let remaining_last_stores = self.remove_unloaded_last_stores(&removed_loads); + let remaining_last_stores = + self.remove_unloaded_last_stores(&removed_loads, &all_terminator_values); self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); - // After possibly removing some instructions we need to map all the instructions - // TODO: check whether we removed any instructions, if we have not skip this check + // After possibly removing some instructions we need to map all the instructions let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); block_order.reverse(); for block in block_order { @@ -737,52 +681,42 @@ impl<'f> PerFunctionContext<'f> { /// Remove any loads whose reference counter is zero. /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { - let mut all_block_params: HashSet = HashSet::default(); - for (block_id, _) in self.blocks.iter() { - let block_params = self.inserter.function.dfg.block_parameters(*block_id); - all_block_params.extend(block_params.iter()); - } - let mut removed_loads = HashMap::default(); - for (_, PerFuncLoadResultContext { uses, load_instruction, block_id, .. }) in self.load_results.iter() + for (_, PerFuncLoadResultContext { uses, load_instruction, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { unreachable!("Should only have a load instruction here"); }; - - // let is_reference_result = self.inserter.function.dfg.value_is_reference(*result); - // If the load result's counter is equal to zero we can safely remove that load instruction. if *uses == 0 { - - // if all_block_params.contains(&address) { - // continue; - // } - if let Some((store_uses, blocks)) = self.store_count.get_mut(&address) { - // println!("have store use"); + if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { *store_uses -= 1; } - // if let Some(block_uses) = blocks.get_mut(block_id) { - // *block_uses -= 1; - // } } - removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); - self.instructions_to_remove.insert(*load_instruction); } } removed_loads } - fn recursively_check_address_in_terminator(&self, return_value: ValueId, store_address: ValueId, is_return_value: &mut bool) { + fn recursively_check_address_in_terminator( + &self, + return_value: ValueId, + store_address: ValueId, + is_return_value: &mut bool, + ) { *is_return_value = return_value == store_address || *is_return_value; let array_const = self.inserter.function.dfg.get_array_constant(return_value); if let Some((values, _)) = array_const { for array_value in values { - self.recursively_check_address_in_terminator(array_value, store_address, is_return_value); + self.recursively_check_address_in_terminator( + array_value, + store_address, + is_return_value, + ); } } } @@ -794,235 +728,98 @@ impl<'f> PerFunctionContext<'f> { fn remove_unloaded_last_stores( &mut self, removed_loads: &HashMap, + all_terminator_values: &HashSet, ) -> HashMap { - let mut all_block_params: HashSet = HashSet::default(); + let mut per_func_block_params: HashSet = HashSet::default(); for (block_id, _) in self.blocks.iter() { let block_params = self.inserter.function.dfg.block_parameters(*block_id); - all_block_params.extend(block_params.iter()); + per_func_block_params.extend(block_params.iter()); } - // let func_params = self.inserter.function.parameters(); let mut remaining_last_stores: HashMap = HashMap::default(); for (block_id, block) in self.blocks.iter() { - self.inserter.map_terminator_in_place(*block_id); - - let block_params = self.inserter.function.dfg.block_parameters(*block_id); for (store_address, store_instruction) in block.last_stores.iter() { if self.instructions_to_remove.contains(store_instruction) { continue; } - let is_reference_param = block_params.contains(store_address); + + let is_used_in_terminator = all_terminator_values.contains(store_address); let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); - let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] - else { - unreachable!("Should only have a store instruction"); - }; let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); // Determine whether any loads that reference this store address // have been removed while cleaning up unused loads. - let remove_load = if is_return { + let loads_removed = if is_return { // Determine whether the last store is used in the return value let mut is_return_value = false; terminator.for_each_value(|return_value| { - is_return_value = return_value == *store_address || is_return_value; + self.recursively_check_address_in_terminator( + return_value, + *store_address, + &mut is_return_value, + ); }); - let mut new_is_return_value = false; - terminator.for_each_value(|return_value| { - self.recursively_check_address_in_terminator(return_value, *store_address, &mut new_is_return_value); - }); - if new_is_return_value != is_return_value { - println!("{}", store_address); - } - - // If the last load of a store is not part of the block with a return terminator, - // we can safely remove this store. - let last_load_not_in_return = self - .last_loads - .get(store_address) - .map(|context| context.block_id != *block_id) - .unwrap_or(true); - // println!("remove store {} in return: {}", store_address, !is_return_value && last_load_not_in_return); - // if let Some((store_uses, blocks)) = self.store_count.get(store_address) { - // dbg!(block_id); - // if let Some(return_block_uses) = blocks.get(&block_id) { - // println!("store uses contains return block {} with {} uses in the return block", block_id, return_block_uses); - // !is_return_value && last_load_not_in_return && *return_block_uses <= 1 - // } else { - // println!("{} with {} uses, !is_return_value && last_load_not_in_return: {}", store_address, store_uses, !is_return_value && last_load_not_in_return); - // !is_return_value && last_load_not_in_return && *store_uses <= 1 - // } - // // println!("{} with {} uses, !is_return_value && last_load_not_in_return: {}", store_address, store_uses, !is_return_value && last_load_not_in_return); - // // dbg!(blocks.clone()); - // // !is_return_value && last_load_not_in_return && *store_uses <= 1 - // // !is_return_value && last_load_not_in_return && *store_uses == 0 || store_uses - 1 == 0 - // // !is_return_value && last_load_not_in_return && *store_uses > 1 - // } else { - // !is_return_value && last_load_not_in_return - // } - // dbg!(self.load_instruction_locations.clone()); - - - let mut max_load_index: usize = 0; - if let Some(x) = self.load_instruction_locations.get(&(*store_address, *block_id)) { - // x.value - // if store_address.to_usize() == 8823 { - // dbg!(x.values()); - // } - for index in x.values() { - if *index > max_load_index { - max_load_index = *index; - } - } - } - let store_index = self.inserter.function.dfg[*block_id].instructions().iter().position(|id| *id == *store_instruction).unwrap(); - // if store_index > max_load_index { - // dbg!(store_index); - // dbg!(max_load_index); - // let (uses, blocks) = self.store_count.get(store_address).unwrap(); - // let uses_in_block = blocks.get(block_id).unwrap(); - // println!("return_block {} store_address {} store_index {} max_load_index {} uses_in_block {} uses {}", block_id, store_address, store_index, max_load_index, uses_in_block, uses); - // } - // let is_value_param = all_block_params.contains(&value); - - // if store_index == 1 && max_load_index == 0 && block_id.to_usize() == 159 { - // dbg!(store_address); - // dbg!(value); - // dbg!(is_value_param); - // dbg!(is_reference_param); - // dbg!(all_block_params.contains(store_address)); - // dbg!(self.load_instruction_locations.get(&(*store_address, *block_id)).clone()); - // } - // let (uses, blocks) = self.store_count.get(store_address).unwrap(); - // let uses_in_block = blocks.get(block_id).unwrap(); - // !is_return_value && last_load_not_in_return && max_load_index == 0 - // TODO: we should not need this uses in block, as we check whether the reference is used in the terminator - // If the max_load_index is zero we may have constant folding out a load?? - // !is_return_value && last_load_not_in_return && store_index > max_load_index && max_load_index != 0 - !is_return_value && store_index > max_load_index && max_load_index != 0 - - // false - } else if let (Some(context), Some(loads_removed_counter), Some((store_uses, _))) = - (self.last_loads.get(store_address), removed_loads.get(store_address), self.store_count.get(store_address)) - { + // If we are in a return terminator, and the last load of an address + // comes after a store to that address, we can safely remove that store. + let store_after_load = if let Some(max_load_index) = + self.return_block_load_locations.get(&(*store_address, *block_id)) + { + let store_index = self.inserter.function.dfg[*block_id] + .instructions() + .iter() + .position(|id| *id == *store_instruction) + .expect("Store instruction should exist in the return block"); + // TODO: Want to get rid of this usage of store_uses + store_index > *max_load_index && *store_uses == 1 + } else { + // Otherwise there is no load in this block + true + }; + !is_return_value && store_after_load + } else if let (Some(context), Some(loads_removed_counter), Some(store_uses)) = ( + self.last_loads.get(store_address), + removed_loads.get(store_address), + self.store_count.get(store_address), + ) { // `last_loads` contains the total number of loads for a given load address // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. - - // if context.num_loads == *loads_removed_counter { - // dbg!(loads_removed_counter); - // dbg!(store_uses); - // // dbg!(self.store_count.get(store_address)); - // dbg!(store_address); - // } - - (store_uses - 1) == 0 && context.num_loads == *loads_removed_counter - // context.num_loads == *loads_removed_counter && store_uses == loads_removed_counter - // context.num_loads == *loads_removed_counter && (store_uses - 1) == *loads_removed_counter - // if context.num_loads == *loads_removed_counter { - // } - - // false + // TODO: Want to get rid of this usage of store_uses + context.num_loads == *loads_removed_counter + && !is_used_in_terminator + && *store_uses == 1 } else { - let last_load_exists = self.last_loads.get(store_address).is_some(); - if let Some((store_uses, _)) = self.store_count.get(store_address) { - // if last_load_exists { - // dbg!(self.store_count.get(store_address)); - // } - // if !last_load_exists && *store_uses == 0 { - // println!("!last_load_exists && *store_uses == 0 for {}", store_address); - // } - !last_load_exists && *store_uses <= 1 - } else { - // println!("!last_load_exists for {}", store_address); - !last_load_exists - } - // TODO: getting a failure here - // self.store_count.get(store_address) - // if !last_load_exists { - // println!("{} num uses: {}", store_address, self.store_count.get(store_address).unwrap()); - // } - // !last_load_exists - // false - // Otherwise just check whether a load exists at all for this store address - // self.last_loads.get(store_address).is_none() + self.last_loads.get(store_address).is_none() && !is_used_in_terminator }; - if remove_load == true && store_address.to_usize() == 24100 { - println!("got here"); - } - - let is_used_in_reference_param = + // Extra checks on where a reference can be used aside a load instruction. + // Even if all loads to a reference have been removed + let is_used_as_reference_arg = self.calls_reference_input.get(store_address).is_some(); let is_reference_alias = block .expressions .get(store_address) .map_or(false, |expression| matches!(expression, Expression::Dereference(_))); + let is_reference_param = per_func_block_params.contains(store_address); - - // let is_value_reference = self.inserter.function.dfg.value_is_reference(value); - let is_value_param = all_block_params.contains(&value); - // let is_func_param = func_params.contains(&store_address); - if remove_load + if loads_removed && !is_reference_param - && !is_used_in_reference_param - && !is_reference_alias - // && !is_func_param - // && !is_value_reference - // && !is_value_param - { - // println!("{}", store_address); - // if is_value_param { - // println!("got value alias"); - // } - // let is_all_blocks_ref_param = all_block_params.contains(store_address); - // if is_all_blocks_ref_param { - // println!("removing store that is a block param"); - // } - - // if is_value_param { - // println!("{}", store_address); - // } - // let is_value_reference = self.inserter.function.dfg.value_is_reference(value); - // dbg!(is_value_reference); - // if is_value_reference { - // dbg!(store_address); - // dbg!(is_reference_alias); - // } - // let is_all_blocks_param = all_block_params.contains(store_address); - // if is_all_blocks_param != is_reference_param { - // dbg!(store_address); - // } + && !is_used_as_reference_arg + && !is_reference_alias + { self.instructions_to_remove.insert(*store_instruction); - if let Some((store_uses, blocks)) = self.store_count.get_mut(&store_address) { + if let Some(store_uses) = self.store_count.get_mut(store_address) { if *store_uses != 0 { *store_uses -= 1; } - // if let Some(block_uses) = blocks.get_mut(&block_id) { - // *block_uses -= 1; - // } } if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; } } else if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { - // if is_reference_alias || is_reference_param || is_used_in_reference_param { - // dbg!(store_address); - // } *counter += 1; } else { - // if is_reference_alias || is_reference_param || is_used_in_reference_param { - // dbg!(store_address); - // dbg!(is_reference_alias); - // dbg!(is_reference_param); - // dbg!(is_reference_alias); - // } - // TODO: temp make this 2, we want to account for remaining last stores better - // We do not count stores to reference params as a remaining store for possible removal - // if !is_reference_param { - // remaining_last_stores.insert(*store_address, (*store_instruction, 1)); - // } remaining_last_stores.insert(*store_address, (*store_instruction, 1)); } } @@ -1036,12 +833,6 @@ impl<'f> PerFunctionContext<'f> { removed_loads: &HashMap, remaining_last_stores: &HashMap, ) { - let mut all_block_params: HashSet = HashSet::default(); - for (block_id, _) in self.blocks.iter() { - let block_params = self.inserter.function.dfg.block_parameters(*block_id); - all_block_params.extend(block_params.iter()); - } - // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, uses, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] @@ -1051,14 +842,12 @@ impl<'f> PerFunctionContext<'f> { remaining_last_stores.contains_key(&address) && *uses > 0 }); - // let mut blocks_to_map_instructions = Vec::new(); - // let mut blocks_to_map_terminators = HashSet::default(); for (store_address, (store_instruction, store_counter)) in remaining_last_stores { let Instruction::Store { value, .. } = self.inserter.function.dfg[*store_instruction] else { unreachable!("Should only have a store instruction"); }; - + if let (Some(context), Some(loads_removed_counter)) = (self.last_loads.get(store_address), removed_loads.get(store_address)) { @@ -1068,26 +857,10 @@ impl<'f> PerFunctionContext<'f> { ); } - // let is_reference_value = self.inserter.function.dfg.value_is_reference(value); - // if is_reference_value { - // dbg!(store_counter); - // dbg!(store_address); - // } - - // if all_block_params.contains(store_address) { - // continue; - // } - // We only want to remove last stores referencing a single address. if *store_counter != 0 { continue; } - // let is_used_in_reference_param = - // self.calls_reference_input.get(store_address).is_some(); - // if is_used_in_reference_param { - // dbg!(store_address); - // } - self.instructions_to_remove.insert(*store_instruction); // Map any remaining load results to the value from the removed store @@ -1100,25 +873,10 @@ impl<'f> PerFunctionContext<'f> { if address != *store_address { continue; } - - // if address.to_usize() == 22868 { - // dbg!(context.clone()); - // } - - + // Map the load result to its respective store value + // We will have to map all instructions following this method + // as we do not know what instructions depend upon this result self.inserter.map_value(*result, value); - - // for (instruction, block_id) in &context.instructions_using_result { - // if self.instructions_to_remove.contains(instruction) { - // continue; - // } - // blocks_to_map_instructions.push(*block_id); - // } - - // for (_, block_id) in &context.terminators_using_result { - // blocks_to_map_terminators.insert(*block_id); - // } - self.instructions_to_remove.insert(context.load_instruction); } } @@ -1811,14 +1569,4 @@ mod tests { assert_eq!(count_loads(b1, &main.dfg), 0); assert_eq!(count_loads(b2, &main.dfg), 1); } - - #[test] - fn keep_last_store_with_value_block_param() { - - } - - // TODO: - // 1. Add a test that a removed load result is mapped throughout all the instructions, - // e.g. if the result of a new instruction caused by a removed load is used as a store value - // 2. Add a test that } diff --git a/test_programs/execution_success/references_testing/Nargo.toml b/test_programs/execution_success/references_testing/Nargo.toml new file mode 100644 index 00000000000..ad0781cbd30 --- /dev/null +++ b/test_programs/execution_success/references_testing/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "references_testing" +type = "bin" +authors = [""] +compiler_version = ">=0.33.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/references_testing/src/main.nr b/test_programs/execution_success/references_testing/src/main.nr new file mode 100644 index 00000000000..bb3376e2ff6 --- /dev/null +++ b/test_programs/execution_success/references_testing/src/main.nr @@ -0,0 +1,86 @@ +struct ExampleEvent0 { + value0: Field, + value1: Field, +} + +trait EventInterface { + fn emit(self, _emit: fn[Env](Self) -> ()); +} + +impl EventInterface for ExampleEvent0 { + fn emit(self: ExampleEvent0, _emit: fn[Env](Self) -> ()) { + _emit(self); + } +} + +struct ExampleEvent1 { + value2: u8, + value3: u8, +} + +struct Context { + a: u32, + b: [u32; 3], + log_hashes: BoundedVec, +} + +struct LogHash { + value: Field, + counter: u32, + length: Field, + randomness: Field, +} + +impl Context { + fn emit_raw_log(&mut self, randomness: Field, log: [u8; M], log_hash: Field) { + let log_hash = LogHash { value: log_hash, counter: 0, length: 0, randomness }; + self.log_hashes.push(log_hash); + } +} + +fn compute(_event: Event) -> ([u8; 5], Field) where Event: EventInterface { + ([0 as u8; 5], 0) +} + +fn emit_with_keys( + context: &mut Context, + randomness: Field, + event: Event, + inner_compute: fn(Event) -> ([u8; OB], Field) +) where Event: EventInterface { + let (log, log_hash) = inner_compute(event); + context.emit_raw_log(randomness, log, log_hash); +} + +pub fn encode_event_with_randomness(context: &mut Context, randomness: Field) -> fn[(&mut Context, Field)](Event) -> () where Event: EventInterface { + | e: Event | { + unsafe { + func(context.a); + } + emit_with_keys(context, randomness, e, compute); + } +} + +unconstrained fn func(input: u32) { + let mut var = input; + let ref = &mut &mut var; + + for _ in 0..1 { + **ref = 2; + } + + assert(var == 2); + assert(**ref == 2); +} + +fn main(input: [Field; 4], randomness: Field, context_input: u32) { + let b = [context_input, context_input, context_input]; + let context = Context { a: context_input, b, log_hashes: BoundedVec::new() }; + + let event0 = ExampleEvent0 { value0: input[0], value1: input[1] }; + event0.emit(encode_event_with_randomness(&mut context, randomness)); + + // let event1 = ExampleEvent1 { value2: preimages[2] as u8, value3: preimages[3] as u8 }; + // context +} + From ab0489a275feefee50e47952ab089cc78d6b6ffd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 16:33:01 +0000 Subject: [PATCH 31/58] nargo mft --- .../execution_success/references_testing/src/main.nr | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test_programs/execution_success/references_testing/src/main.nr b/test_programs/execution_success/references_testing/src/main.nr index bb3376e2ff6..8d30489b0cb 100644 --- a/test_programs/execution_success/references_testing/src/main.nr +++ b/test_programs/execution_success/references_testing/src/main.nr @@ -47,12 +47,15 @@ fn emit_with_keys( randomness: Field, event: Event, inner_compute: fn(Event) -> ([u8; OB], Field) -) where Event: EventInterface { +) where Event: EventInterface { let (log, log_hash) = inner_compute(event); context.emit_raw_log(randomness, log, log_hash); } -pub fn encode_event_with_randomness(context: &mut Context, randomness: Field) -> fn[(&mut Context, Field)](Event) -> () where Event: EventInterface { +pub fn encode_event_with_randomness( + context: &mut Context, + randomness: Field +) -> fn[(&mut Context, Field)](Event) -> () where Event: EventInterface { | e: Event | { unsafe { func(context.a); @@ -79,7 +82,6 @@ fn main(input: [Field; 4], randomness: Field, context_input: u32) { let event0 = ExampleEvent0 { value0: input[0], value1: input[1] }; event0.emit(encode_event_with_randomness(&mut context, randomness)); - // let event1 = ExampleEvent1 { value2: preimages[2] as u8, value3: preimages[3] as u8 }; // context } From abc61fb9618c03698c283f5d934fee8b0790249a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 16:34:03 +0000 Subject: [PATCH 32/58] fix missing error --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index a3bf380c496..edb7ba5ed9c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -771,7 +771,11 @@ impl<'f> PerFunctionContext<'f> { .position(|id| *id == *store_instruction) .expect("Store instruction should exist in the return block"); // TODO: Want to get rid of this usage of store_uses - store_index > *max_load_index && *store_uses == 1 + if let Some(store_uses) = self.store_count.get(store_address) { + store_index > *max_load_index && *store_uses == 1 + } else { + store_index > *max_load_index + } } else { // Otherwise there is no load in this block true From bad6d1d24272c44d46d4acc4f9cbc715a74f0a6a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 16:37:05 +0000 Subject: [PATCH 33/58] tiny cleanup --- test_programs/execution_success/references_testing/src/main.nr | 2 -- tooling/nargo/src/ops/compile.rs | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/test_programs/execution_success/references_testing/src/main.nr b/test_programs/execution_success/references_testing/src/main.nr index 8d30489b0cb..43c7ef9b94f 100644 --- a/test_programs/execution_success/references_testing/src/main.nr +++ b/test_programs/execution_success/references_testing/src/main.nr @@ -82,7 +82,5 @@ fn main(input: [Field; 4], randomness: Field, context_input: u32) { let event0 = ExampleEvent0 { value0: input[0], value1: input[1] }; event0.emit(encode_event_with_randomness(&mut context, randomness)); - // let event1 = ExampleEvent1 { value2: preimages[2] as u8, value3: preimages[3] as u8 }; - // context } diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index 81401f66f05..cd9ccf67957 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -36,7 +36,7 @@ pub fn compile_workspace( }) .collect(); let contract_results: Vec> = contract_packages - .iter() + .par_iter() .map(|package| compile_contract(file_manager, parsed_files, package, compile_options)) .collect(); From fc35396fae0ee7a3740b42eab39ae7bf8609ea4b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 16:38:38 +0000 Subject: [PATCH 34/58] add todo --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index edb7ba5ed9c..e61b7e74916 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -664,6 +664,7 @@ impl<'f> PerFunctionContext<'f> { self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); // After possibly removing some instructions we need to map all the instructions + // TODO: Add a check whether any loads were actually removed where we actually need to map values let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); block_order.reverse(); for block in block_order { From 3290dcb9e27532762dcf5812d54c5ac7bdce6e32 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 16:44:16 +0000 Subject: [PATCH 35/58] note that store_uses can be removed --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e61b7e74916..f4516bfe8ac 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -772,8 +772,10 @@ impl<'f> PerFunctionContext<'f> { .position(|id| *id == *store_instruction) .expect("Store instruction should exist in the return block"); // TODO: Want to get rid of this usage of store_uses - if let Some(store_uses) = self.store_count.get(store_address) { - store_index > *max_load_index && *store_uses == 1 + if let Some(_store_uses) = self.store_count.get(store_address) { + store_index > *max_load_index + // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts + // && *store_uses == 1 } else { store_index > *max_load_index } @@ -782,7 +784,7 @@ impl<'f> PerFunctionContext<'f> { true }; !is_return_value && store_after_load - } else if let (Some(context), Some(loads_removed_counter), Some(store_uses)) = ( + } else if let (Some(context), Some(loads_removed_counter), Some(_store_uses)) = ( self.last_loads.get(store_address), removed_loads.get(store_address), self.store_count.get(store_address), @@ -791,9 +793,9 @@ impl<'f> PerFunctionContext<'f> { // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. // TODO: Want to get rid of this usage of store_uses - context.num_loads == *loads_removed_counter - && !is_used_in_terminator - && *store_uses == 1 + context.num_loads == *loads_removed_counter && !is_used_in_terminator + // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts + // && *store_uses == 1 } else { self.last_loads.get(store_address).is_none() && !is_used_in_terminator }; From 00f2f0eac6b52fcdd2ddde5d2b78d34bd9a82d72 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 17:36:28 +0000 Subject: [PATCH 36/58] fix tests --- compiler/noirc_evaluator/src/ssa.rs | 2 +- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 98 +++++++++++++------ .../references_testing/Prover.toml | 3 + 3 files changed, 72 insertions(+), 31 deletions(-) create mode 100644 test_programs/execution_success/references_testing/Prover.toml diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index ad6645df228..ac6b42bbd19 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -414,7 +414,7 @@ impl SsaBuilder { mut self, pass: fn(Ssa) -> Result, msg: &str, - ) -> Result { +) -> Result { self.ssa = time(msg, self.print_codegen_timings, || pass(self.ssa))?; Ok(self.print(msg)) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index f4516bfe8ac..a812b45c5d9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -411,7 +411,19 @@ impl<'f> PerFunctionContext<'f> { if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); if let Some(context) = self.load_results.get_mut(&value) { - context.uses -= 1; + if context.uses > 0 { + context.uses -= 1; + } + } + if let Some(context) = self.load_results.get_mut(&address) { + if context.uses > 0 { + context.uses -= 1; + } + } + if let Some(store_uses) = self.store_count.get_mut(&value) { + if *store_uses != 0 { + *store_uses -= 1; + } } if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { @@ -424,9 +436,22 @@ impl<'f> PerFunctionContext<'f> { if let Some(known_value) = known_value { let known_value_is_address = known_value == address; if known_value_is_address { + // TODO: move all of this into a utility method for when removing an instruction self.instructions_to_remove.insert(instruction); + if let Some(context) = self.load_results.get_mut(&value) { + if context.uses > 0 { + context.uses -= 1; + } + } if let Some(context) = self.load_results.get_mut(&address) { - context.uses -= 1; + if context.uses > 0 { + context.uses -= 1; + } + } + if let Some(store_uses) = self.store_count.get_mut(&value) { + if *store_uses != 0 { + *store_uses -= 1; + } } if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { @@ -683,12 +708,13 @@ impl<'f> PerFunctionContext<'f> { /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { let mut removed_loads = HashMap::default(); - for (_, PerFuncLoadResultContext { uses, load_instruction, .. }) in self.load_results.iter() + for (result, PerFuncLoadResultContext { uses, load_instruction, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { unreachable!("Should only have a load instruction here"); }; + println!("{result} with {uses} uses"); // If the load result's counter is equal to zero we can safely remove that load instruction. if *uses == 0 { if let Some(store_uses) = self.store_count.get_mut(&address) { @@ -760,31 +786,36 @@ impl<'f> PerFunctionContext<'f> { &mut is_return_value, ); }); - + println!("{is_return_value} is used in return {store_address}"); // If we are in a return terminator, and the last load of an address // comes after a store to that address, we can safely remove that store. - let store_after_load = if let Some(max_load_index) = - self.return_block_load_locations.get(&(*store_address, *block_id)) - { - let store_index = self.inserter.function.dfg[*block_id] - .instructions() - .iter() - .position(|id| *id == *store_instruction) - .expect("Store instruction should exist in the return block"); - // TODO: Want to get rid of this usage of store_uses - if let Some(_store_uses) = self.store_count.get(store_address) { - store_index > *max_load_index - // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts - // && *store_uses == 1 - } else { - store_index > *max_load_index - } - } else { - // Otherwise there is no load in this block - true - }; - !is_return_value && store_after_load - } else if let (Some(context), Some(loads_removed_counter), Some(_store_uses)) = ( + // let store_after_load = if let Some(max_load_index) = + // self.return_block_load_locations.get(&(*store_address, *block_id)) + // { + // let store_index = self.inserter.function.dfg[*block_id] + // .instructions() + // .iter() + // .position(|id| *id == *store_instruction) + // .expect("Store instruction should exist in the return block"); + // // TODO: Want to get rid of this usage of store_uses + // if let Some(store_uses) = self.store_count.get(store_address) { + // store_index > *max_load_index + // // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts + // && *store_uses == 1 + // } else { + // store_index > *max_load_index + // } + // } else { + // // Otherwise there is no load in this block + // true + // }; + let last_load_not_in_return = self + .last_loads + .get(store_address) + .map(|context| context.block_id != *block_id) + .unwrap_or(true); + !is_return_value && last_load_not_in_return + } else if let (Some(context), Some(loads_removed_counter), Some(store_uses)) = ( self.last_loads.get(store_address), removed_loads.get(store_address), self.store_count.get(store_address), @@ -793,11 +824,13 @@ impl<'f> PerFunctionContext<'f> { // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. // TODO: Want to get rid of this usage of store_uses - context.num_loads == *loads_removed_counter && !is_used_in_terminator + context.num_loads == *loads_removed_counter + // && !is_used_in_terminator // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts // && *store_uses == 1 } else { - self.last_loads.get(store_address).is_none() && !is_used_in_terminator + self.last_loads.get(store_address).is_none() + // && !is_used_in_terminator }; // Extra checks on where a reference can be used aside a load instruction. @@ -814,6 +847,7 @@ impl<'f> PerFunctionContext<'f> { && !is_reference_param && !is_used_as_reference_arg && !is_reference_alias + // && !is_used_in_terminator { self.instructions_to_remove.insert(*store_instruction); if let Some(store_uses) = self.store_count.get_mut(store_address) { @@ -840,13 +874,16 @@ impl<'f> PerFunctionContext<'f> { removed_loads: &HashMap, remaining_last_stores: &HashMap, ) { + dbg!(remaining_last_stores.clone()); // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, uses, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { unreachable!("Should only have a load instruction here"); }; - remaining_last_stores.contains_key(&address) && *uses > 0 + // remaining_last_stores.contains_key(&address) + // && *uses > 0 + true }); for (store_address, (store_instruction, store_counter)) in remaining_last_stores { @@ -863,7 +900,7 @@ impl<'f> PerFunctionContext<'f> { "The number of loads removed should not be more than all loads" ); } - + println!("{store_address} with {store_counter} uses"); // We only want to remove last stores referencing a single address. if *store_counter != 0 { continue; @@ -880,6 +917,7 @@ impl<'f> PerFunctionContext<'f> { if address != *store_address { continue; } + println!("{result} removed as remaining load result"); // Map the load result to its respective store value // We will have to map all instructions following this method // as we do not know what instructions depend upon this result diff --git a/test_programs/execution_success/references_testing/Prover.toml b/test_programs/execution_success/references_testing/Prover.toml new file mode 100644 index 00000000000..ef34b9eba70 --- /dev/null +++ b/test_programs/execution_success/references_testing/Prover.toml @@ -0,0 +1,3 @@ +input = [0, 1, 2, 3] +context_input = 4 +randomness = 5 From e60d4ce854bf7c55f7151b5d21e29497564e3898 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 17:36:36 +0000 Subject: [PATCH 37/58] cargo fmt --- compiler/noirc_evaluator/src/ssa.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index ac6b42bbd19..ad6645df228 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -414,7 +414,7 @@ impl SsaBuilder { mut self, pass: fn(Ssa) -> Result, msg: &str, -) -> Result { + ) -> Result { self.ssa = time(msg, self.print_codegen_timings, || pass(self.ssa))?; Ok(self.print(msg)) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index a812b45c5d9..dab6a58f1c6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -708,7 +708,8 @@ impl<'f> PerFunctionContext<'f> { /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { let mut removed_loads = HashMap::default(); - for (result, PerFuncLoadResultContext { uses, load_instruction, .. }) in self.load_results.iter() + for (result, PerFuncLoadResultContext { uses, load_instruction, .. }) in + self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { @@ -824,12 +825,12 @@ impl<'f> PerFunctionContext<'f> { // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. // TODO: Want to get rid of this usage of store_uses - context.num_loads == *loads_removed_counter + context.num_loads == *loads_removed_counter // && !is_used_in_terminator // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts // && *store_uses == 1 } else { - self.last_loads.get(store_address).is_none() + self.last_loads.get(store_address).is_none() // && !is_used_in_terminator }; @@ -847,7 +848,7 @@ impl<'f> PerFunctionContext<'f> { && !is_reference_param && !is_used_as_reference_arg && !is_reference_alias - // && !is_used_in_terminator + // && !is_used_in_terminator { self.instructions_to_remove.insert(*store_instruction); if let Some(store_uses) = self.store_count.get_mut(store_address) { @@ -881,7 +882,7 @@ impl<'f> PerFunctionContext<'f> { else { unreachable!("Should only have a load instruction here"); }; - // remaining_last_stores.contains_key(&address) + // remaining_last_stores.contains_key(&address) // && *uses > 0 true }); From f90fd95015409fc36dff6c5de2aea8230cfd3e79 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 17:38:10 +0000 Subject: [PATCH 38/58] remove debugging prints --- compiler/noirc_driver/src/lib.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 9 +++------ tooling/nargo/src/ops/compile.rs | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 2e43dfcb527..8004a77bd14 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -421,7 +421,7 @@ fn compile_contract_inner( let mut functions = Vec::new(); let mut errors = Vec::new(); let mut warnings = Vec::new(); - + println!("contract {}", contract.name.clone()); for contract_function in &contract.functions { let function_id = contract_function.function_id; let is_entry_point = contract_function.is_entry_point; diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index dab6a58f1c6..1c99a3f9926 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -875,16 +875,13 @@ impl<'f> PerFunctionContext<'f> { removed_loads: &HashMap, remaining_last_stores: &HashMap, ) { - dbg!(remaining_last_stores.clone()); // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, uses, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { unreachable!("Should only have a load instruction here"); }; - // remaining_last_stores.contains_key(&address) - // && *uses > 0 - true + remaining_last_stores.contains_key(&address) && *uses > 0 }); for (store_address, (store_instruction, store_counter)) in remaining_last_stores { @@ -901,7 +898,7 @@ impl<'f> PerFunctionContext<'f> { "The number of loads removed should not be more than all loads" ); } - println!("{store_address} with {store_counter} uses"); + // We only want to remove last stores referencing a single address. if *store_counter != 0 { continue; @@ -918,7 +915,7 @@ impl<'f> PerFunctionContext<'f> { if address != *store_address { continue; } - println!("{result} removed as remaining load result"); + // Map the load result to its respective store value // We will have to map all instructions following this method // as we do not know what instructions depend upon this result diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index cd9ccf67957..81401f66f05 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -36,7 +36,7 @@ pub fn compile_workspace( }) .collect(); let contract_results: Vec> = contract_packages - .par_iter() + .iter() .map(|package| compile_contract(file_manager, parsed_files, package, compile_options)) .collect(); From 2c32703e9c6b1de1b840534dbe34f20a17f347ff Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 13 Sep 2024 21:05:03 +0000 Subject: [PATCH 39/58] to get everything passing with aztec-packages, remove_unused_loads_and_stores unit test now fails, but we arrive at the ideal ssa after another run, we must be missing some usage of a store --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 205 ++++++++++++------ 1 file changed, 133 insertions(+), 72 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 1c99a3f9926..96f44fc91a6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -355,9 +355,6 @@ impl<'f> PerFunctionContext<'f> { if let Some(value) = references.get_known_value(address) { self.inserter.map_value(result, value); self.instructions_to_remove.insert(instruction); - if let Some(context) = self.load_results.get_mut(&address) { - context.uses -= 1; - } if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { *store_uses -= 1; @@ -406,10 +403,15 @@ impl<'f> PerFunctionContext<'f> { self.check_array_aliasing(references, value); - // If there was another store to this instruction without any (unremoved) loads or + // If there was another store to this address without any (unremoved) loads or // function calls in-between, we can remove the previous store. if let Some(last_store) = references.last_stores.get(&address) { self.instructions_to_remove.insert(*last_store); + let Instruction::Store { address, value } = + self.inserter.function.dfg[*last_store] + else { + panic!("Must have a Store instruction here"); + }; if let Some(context) = self.load_results.get_mut(&value) { if context.uses > 0 { context.uses -= 1; @@ -437,35 +439,49 @@ impl<'f> PerFunctionContext<'f> { let known_value_is_address = known_value == address; if known_value_is_address { // TODO: move all of this into a utility method for when removing an instruction + // the utility method should have a seen values state to guarantee we do not decrement more than needed self.instructions_to_remove.insert(instruction); - if let Some(context) = self.load_results.get_mut(&value) { - if context.uses > 0 { - context.uses -= 1; - } - } - if let Some(context) = self.load_results.get_mut(&address) { - if context.uses > 0 { - context.uses -= 1; - } - } - if let Some(store_uses) = self.store_count.get_mut(&value) { - if *store_uses != 0 { - *store_uses -= 1; - } - } + // if let Some(context) = self.load_results.get_mut(&address) { + // if context.uses > 0 { + // context.uses -= 1; + // } + // } + // TODO: This is causing breakages in card game contracts + // if let Some(context) = self.load_results.get_mut(&value) { + // if context.uses > 0 { + // context.uses -= 1; + // } + // } + // value == address so we should only reduce the use once here if let Some(store_uses) = self.store_count.get_mut(&address) { if *store_uses != 0 { *store_uses -= 1; } } + } else { + references.last_stores.insert(address, instruction); + + let num_stores = self.store_count.get(&address).copied().unwrap_or(1); + self.store_count.insert(address, num_stores); } + } else { + references.last_stores.insert(address, instruction); + + let num_stores = self.store_count.get(&address).copied().unwrap_or(1); + self.store_count.insert(address, num_stores); } + let expression = + references.expressions.entry(address).or_insert(Expression::Other(address)); + // Make sure this load result is marked an alias to itself + if let Some(aliases) = references.aliases.get_mut(expression) { + // If we have an alias set, add to the set + aliases.insert(address); + } else { + // Otherwise, create a new alias set containing just the load result + references.aliases.insert(Expression::Other(address), AliasSet::known(address)); + } references.set_known_value(address, value); - references.last_stores.insert(address, instruction); - - let num_stores = self.store_count.get(&address).copied().unwrap_or(1); - self.store_count.insert(address, num_stores); } Instruction::Allocate => { // Register the new reference @@ -533,6 +549,23 @@ impl<'f> PerFunctionContext<'f> { } } + fn recursively_add_call_arg(&mut self, arg: ValueId, references: &mut Block) { + let arg = self.inserter.function.dfg.resolve(arg); + let aliases = references.get_aliases_for_value(arg); + aliases.for_each(|alias| { + self.calls_reference_input.insert(alias); + }); + if let Some((elements, _)) = self.inserter.function.dfg.get_array_constant(arg) { + for value in elements.iter() { + self.recursively_add_call_arg(*value, references); + } + let aliases = references.collect_all_aliases(elements); + aliases.for_each(|alias| { + self.calls_reference_input.insert(alias); + }); + } + } + fn check_array_aliasing(&self, references: &mut Block, array: ValueId) { if let Some((elements, typ)) = self.inserter.function.dfg.get_array_constant(array) { if Self::contains_references(&typ) { @@ -708,14 +741,13 @@ impl<'f> PerFunctionContext<'f> { /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { let mut removed_loads = HashMap::default(); - for (result, PerFuncLoadResultContext { uses, load_instruction, .. }) in + for (result, PerFuncLoadResultContext { uses, load_instruction, block_id, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] else { unreachable!("Should only have a load instruction here"); }; - println!("{result} with {uses} uses"); // If the load result's counter is equal to zero we can safely remove that load instruction. if *uses == 0 { if let Some(store_uses) = self.store_count.get_mut(&address) { @@ -723,6 +755,13 @@ impl<'f> PerFunctionContext<'f> { *store_uses -= 1; } } + if let Some(store_uses) = self.store_count.get_mut(result) { + if *store_uses != 0 { + *store_uses -= 1; + } + } + self.return_block_load_locations.remove(&(address, *block_id)); + removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); self.instructions_to_remove.insert(*load_instruction); } @@ -763,10 +802,16 @@ impl<'f> PerFunctionContext<'f> { let block_params = self.inserter.function.dfg.block_parameters(*block_id); per_func_block_params.extend(block_params.iter()); } + let func_params = self.inserter.function.parameters(); let mut remaining_last_stores: HashMap = HashMap::default(); for (block_id, block) in self.blocks.iter() { for (store_address, store_instruction) in block.last_stores.iter() { + let Instruction::Store { value, .. } = + self.inserter.function.dfg[*store_instruction] + else { + panic!("Should have Store instruction"); + }; if self.instructions_to_remove.contains(store_instruction) { continue; } @@ -787,35 +832,23 @@ impl<'f> PerFunctionContext<'f> { &mut is_return_value, ); }); - println!("{is_return_value} is used in return {store_address}"); + // If we are in a return terminator, and the last load of an address // comes after a store to that address, we can safely remove that store. - // let store_after_load = if let Some(max_load_index) = - // self.return_block_load_locations.get(&(*store_address, *block_id)) - // { - // let store_index = self.inserter.function.dfg[*block_id] - // .instructions() - // .iter() - // .position(|id| *id == *store_instruction) - // .expect("Store instruction should exist in the return block"); - // // TODO: Want to get rid of this usage of store_uses - // if let Some(store_uses) = self.store_count.get(store_address) { - // store_index > *max_load_index - // // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts - // && *store_uses == 1 - // } else { - // store_index > *max_load_index - // } - // } else { - // // Otherwise there is no load in this block - // true - // }; - let last_load_not_in_return = self - .last_loads - .get(store_address) - .map(|context| context.block_id != *block_id) - .unwrap_or(true); - !is_return_value && last_load_not_in_return + let store_after_load = if let Some(max_load_index) = + self.return_block_load_locations.get(&(*store_address, *block_id)) + { + let store_index = self.inserter.function.dfg[*block_id] + .instructions() + .iter() + .position(|id| *id == *store_instruction) + .expect("Store instruction should exist in the return block"); + store_index > *max_load_index + } else { + // Otherwise there is no load in this block + true + }; + !is_return_value && store_after_load } else if let (Some(context), Some(loads_removed_counter), Some(store_uses)) = ( self.last_loads.get(store_address), removed_loads.get(store_address), @@ -824,38 +857,65 @@ impl<'f> PerFunctionContext<'f> { // `last_loads` contains the total number of loads for a given load address // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. - // TODO: Want to get rid of this usage of store_uses context.num_loads == *loads_removed_counter - // && !is_used_in_terminator + && !is_used_in_terminator + // TODO: We want to get rid of this usage of store_uses // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts - // && *store_uses == 1 + // The only failing contract is card_game_contract + && *store_uses == 1 } else { - self.last_loads.get(store_address).is_none() - // && !is_used_in_terminator + self.last_loads.get(store_address).is_none() && !is_used_in_terminator }; // Extra checks on where a reference can be used aside a load instruction. - // Even if all loads to a reference have been removed - let is_used_as_reference_arg = - self.calls_reference_input.get(store_address).is_some(); - let is_reference_alias = block - .expressions - .get(store_address) - .map_or(false, |expression| matches!(expression, Expression::Dereference(_))); - let is_reference_param = per_func_block_params.contains(store_address); - - if loads_removed - && !is_reference_param - && !is_used_as_reference_arg - && !is_reference_alias - // && !is_used_in_terminator - { + // 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. + let reference_parameters = func_params + .iter() + .filter(|param| self.inserter.function.dfg.value_is_reference(**param)) + .collect::>(); + + let mut store_alias_used = false; + 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) { + store_alias_used = true; + } + + let allocation_aliases_parameter = + aliases.any(|alias| per_func_block_params.contains(&alias)); + if allocation_aliases_parameter == Some(true) { + store_alias_used = true; + } + + let allocation_aliases_parameter = + aliases.any(|alias| self.calls_reference_input.contains(&alias)); + if allocation_aliases_parameter == Some(true) { + store_alias_used = true; + } + + let allocation_aliases_parameter = + aliases.any(|alias| all_terminator_values.contains(&alias)); + if allocation_aliases_parameter == Some(true) { + store_alias_used = true; + } + } + } + + if loads_removed && !store_alias_used { self.instructions_to_remove.insert(*store_instruction); if let Some(store_uses) = self.store_count.get_mut(store_address) { if *store_uses != 0 { *store_uses -= 1; } } + if let Some(store_uses) = self.store_count.get_mut(&value) { + if *store_uses != 0 { + *store_uses -= 1; + } + } if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; } @@ -1472,6 +1532,7 @@ mod tests { // We expect the same result as above. let ssa = ssa.mem2reg(); + println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 4); From 6200597660a9f2b950ddf7c6310d5963c1778264 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 16 Sep 2024 16:38:34 +0000 Subject: [PATCH 40/58] add mut in test --- test_programs/execution_success/references_testing/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/execution_success/references_testing/src/main.nr b/test_programs/execution_success/references_testing/src/main.nr index 43c7ef9b94f..3c5865a04b0 100644 --- a/test_programs/execution_success/references_testing/src/main.nr +++ b/test_programs/execution_success/references_testing/src/main.nr @@ -78,7 +78,7 @@ unconstrained fn func(input: u32) { fn main(input: [Field; 4], randomness: Field, context_input: u32) { let b = [context_input, context_input, context_input]; - let context = Context { a: context_input, b, log_hashes: BoundedVec::new() }; + let mut context = Context { a: context_input, b, log_hashes: BoundedVec::new() }; let event0 = ExampleEvent0 { value0: input[0], value1: input[1] }; event0.emit(encode_event_with_randomness(&mut context, randomness)); From 8232dae4ef928b80d82f7460ac0548ac55f81592 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 16 Sep 2024 20:27:47 +0000 Subject: [PATCH 41/58] remove last usage of store uses and add unit test for store only used as an alias --- compiler/noirc_evaluator/src/ssa.rs | 2 +- .../src/ssa/function_builder/mod.rs | 2 +- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 147 +++++++++++++++--- .../references_testing/src/main.nr | 2 +- 4 files changed, 131 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index ad6645df228..a25d7568388 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -421,7 +421,7 @@ impl SsaBuilder { fn print(mut self, msg: &str) -> Self { if self.print_ssa_passes { - self.ssa.normalize_ids(); + // self.ssa.normalize_ids(); println!("{msg}\n{}", self.ssa); } self diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 04d4e893bf8..06e5e18dda5 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -235,7 +235,7 @@ impl FunctionBuilder { if operator != BinaryOp::Shl && operator != BinaryOp::Shr { assert_eq!( lhs_type, rhs_type, - "ICE - Binary instruction operands must have the same type" + "ICE - Binary instruction operands must have the same type {lhs} and {rhs}, {operator}" ); } let instruction = Instruction::Binary(Binary { lhs, rhs, operator }); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 96f44fc91a6..b0559fd8f1f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -471,16 +471,16 @@ impl<'f> PerFunctionContext<'f> { self.store_count.insert(address, num_stores); } - let expression = - references.expressions.entry(address).or_insert(Expression::Other(address)); - // Make sure this load result is marked an alias to itself - if let Some(aliases) = references.aliases.get_mut(expression) { - // If we have an alias set, add to the set - aliases.insert(address); - } else { - // Otherwise, create a new alias set containing just the load result - references.aliases.insert(Expression::Other(address), AliasSet::known(address)); + 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.calls_reference_input.insert(alias); + }); + } + } } + references.set_known_value(address, value); } Instruction::Allocate => { @@ -721,6 +721,7 @@ impl<'f> PerFunctionContext<'f> { self.remove_unloaded_last_stores(&removed_loads, &all_terminator_values); self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); + // println!("{}", self.inserter.function); // After possibly removing some instructions we need to map all the instructions // TODO: Add a check whether any loads were actually removed where we actually need to map values let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); @@ -812,7 +813,9 @@ impl<'f> PerFunctionContext<'f> { else { panic!("Should have Store instruction"); }; + if self.instructions_to_remove.contains(store_instruction) { + // println!("already set to remove"); continue; } @@ -833,8 +836,8 @@ impl<'f> PerFunctionContext<'f> { ); }); - // If we are in a return terminator, and the last load of an address - // comes after a store to that address, we can safely remove that store. + // If we are in a return terminator, and the last loads of a reference + // come before a store to that reference, we can safely remove that store. let store_after_load = if let Some(max_load_index) = self.return_block_load_locations.get(&(*store_address, *block_id)) { @@ -848,7 +851,7 @@ impl<'f> PerFunctionContext<'f> { // Otherwise there is no load in this block true }; - !is_return_value && store_after_load + store_after_load } else if let (Some(context), Some(loads_removed_counter), Some(store_uses)) = ( self.last_loads.get(store_address), removed_loads.get(store_address), @@ -858,13 +861,8 @@ impl<'f> PerFunctionContext<'f> { // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. context.num_loads == *loads_removed_counter - && !is_used_in_terminator - // TODO: We want to get rid of this usage of store_uses - // TODO: Without this `store_uses` I am getting failures in aztec-packages contracts - // The only failing contract is card_game_contract - && *store_uses == 1 } else { - self.last_loads.get(store_address).is_none() && !is_used_in_terminator + self.last_loads.get(store_address).is_none() }; // Extra checks on where a reference can be used aside a load instruction. @@ -963,6 +961,7 @@ impl<'f> PerFunctionContext<'f> { if *store_counter != 0 { continue; } + // println!("remaining last stores: store {value} at {store_address}"); self.instructions_to_remove.insert(*store_instruction); // Map any remaining load results to the value from the removed store @@ -1269,7 +1268,7 @@ mod tests { // return // } let ssa = ssa.mem2reg(); - + println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 2); @@ -1673,4 +1672,114 @@ mod tests { assert_eq!(count_loads(b1, &main.dfg), 0); assert_eq!(count_loads(b2, &main.dfg), 1); } + + #[test] + fn keep_unused_store_used_as_alias_across_blocks() { + // acir(inline) fn main f0 { + // b0(v0: u32): + // v1 = allocate + // store u32 0 at v1 + // v3 = allocate + // store v1 at v3 + // v4 = allocate + // store v0 at v4 + // v5 = allocate + // store v4 at v5 + // jmp b1(u32 0) + // b1(v6: u32): + // v7 = eq v6, u32 0 + // jmpif v7 then: b2, else: b3 + // b2(): + // v8 = load v5 + // store v8 at u2 2 + // v11 = add v6, u32 1 + // jmp b1(v11) + // b3(): + // v12 = load v4 + // constrain v12 == u2 2 + // v13 = load v5 + // v14 = load v13 + // constrain v14 == u2 2 + // v15 = load v3 + // v16 = load v15 + // v18 = lt v16, u32 4 + // constrain v18 == u32 1 + // return + // } + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + + let v0 = builder.add_parameter(Type::unsigned(32)); + + let v1 = builder.insert_allocate(Type::unsigned(32)); + let zero = builder.numeric_constant(0u128, Type::unsigned(32)); + builder.insert_store(v1, zero); + + let v1_type = builder.type_of_value(v1); + let v3 = builder.insert_allocate(v1_type.clone()); + builder.insert_store(v3, v1); + + let v4 = builder.insert_allocate(Type::unsigned(32)); + builder.insert_store(v4, v0); + + let v5 = builder.insert_allocate(Type::Reference(Arc::new(Type::unsigned(32)))); + builder.insert_store(v5, v4); + + let b1 = builder.insert_block(); + builder.terminate_with_jmp(b1, vec![zero]); + builder.switch_to_block(b1); + + let v6 = builder.add_block_parameter(b1, Type::unsigned(32)); + let is_zero = builder.insert_binary(v6, BinaryOp::Eq, zero); + + let b2 = builder.insert_block(); + let b3 = builder.insert_block(); + builder.terminate_with_jmpif(is_zero, b2, b3); + + builder.switch_to_block(b2); + let v4_type = builder.type_of_value(v4); + // let v0_type = builder.type_of_value(v4); + let v8 = builder.insert_load(v5, v4_type); + let two = builder.numeric_constant(2u128, Type::unsigned(2)); + builder.insert_store(v8, two); + let one = builder.numeric_constant(1u128, Type::unsigned(32)); + let v11 = builder.insert_binary(v6, BinaryOp::Add, one); + builder.terminate_with_jmp(b1, vec![v11]); + + builder.switch_to_block(b3); + + let v12 = builder.insert_load(v4, Type::unsigned(32)); + builder.insert_constrain(v12, two, None); + + let v3_type = builder.type_of_value(v3); + let v13 = builder.insert_load(v5, v3_type); + let v14 = builder.insert_load(v13, Type::unsigned(32)); + builder.insert_constrain(v14, two, None); + + let v15 = builder.insert_load(v3, v1_type); + let v16 = builder.insert_load(v15, Type::unsigned(32)); + let four = builder.numeric_constant(4u128, Type::unsigned(32)); + let less_than_four = builder.insert_binary(v16, BinaryOp::Lt, four); + builder.insert_constrain(less_than_four, one, None); + + builder.terminate_with_return(vec![]); + let ssa = builder.finish(); + + // We expect the same result as above. + let ssa = ssa.mem2reg(); + println!("{}", ssa); + let main = ssa.main(); + + // We expect all the stores to remain. + // The references in b0 are aliased and those are aliases may never be stored to again, + // but they are loaded from and used in later instructions. + // We need to make sure that the store of the address being aliased, is not removed from the program. + assert_eq!(count_stores(main.entry_block(), &main.dfg), 4); + // The store inside of the loop should remain + assert_eq!(count_stores(b2, &main.dfg), 1); + + // We expect the loads to remain the same + assert_eq!(count_loads(b2, &main.dfg), 1); + assert_eq!(count_loads(b3, &main.dfg), 5); + } } diff --git a/test_programs/execution_success/references_testing/src/main.nr b/test_programs/execution_success/references_testing/src/main.nr index 3c5865a04b0..c740bdff662 100644 --- a/test_programs/execution_success/references_testing/src/main.nr +++ b/test_programs/execution_success/references_testing/src/main.nr @@ -32,7 +32,7 @@ struct LogHash { } impl Context { - fn emit_raw_log(&mut self, randomness: Field, log: [u8; M], log_hash: Field) { + fn emit_raw_log(&mut self, randomness: Field, _log: [u8; M], log_hash: Field) { let log_hash = LogHash { value: log_hash, counter: 0, length: 0, randomness }; self.log_hashes.push(log_hash); } From 7cc78dc00532cd059e300a15c8a60213fd47f195 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 16 Sep 2024 22:04:44 +0000 Subject: [PATCH 42/58] improved checks --- compiler/noirc_evaluator/src/ssa.rs | 2 +- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 171 ++++-------------- 2 files changed, 37 insertions(+), 136 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index a25d7568388..ad6645df228 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -421,7 +421,7 @@ impl SsaBuilder { fn print(mut self, msg: &str) -> Self { if self.print_ssa_passes { - // self.ssa.normalize_ids(); + self.ssa.normalize_ids(); println!("{msg}\n{}", self.ssa); } self diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index b0559fd8f1f..da11fc4250e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -113,7 +113,7 @@ struct PerFunctionContext<'f> { /// /// 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, + 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. @@ -126,11 +126,10 @@ struct PerFunctionContext<'f> { /// This is needed to determine whether we can remove a store. calls_reference_input: HashSet, - // The map of block ids represents the blocks where a store was used - // And the usage per block - // TODO: Get rid of this map. This is being used for debugging - // as we are not catching certain usages of a store that we should be - store_count: HashMap, + /// 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>, // The index of the last load instruction in a given block return_block_load_locations: HashMap<(ValueId, BasicBlockId), usize>, @@ -188,11 +187,11 @@ impl<'f> PerFunctionContext<'f> { post_order, inserter: FunctionInserter::new(function), blocks: BTreeMap::new(), - instructions_to_remove: BTreeSet::new(), + instructions_to_remove: HashSet::default(), last_loads: HashMap::default(), load_results: HashMap::default(), calls_reference_input: HashSet::default(), - store_count: HashMap::default(), + aliased_references: HashMap::default(), return_block_load_locations: HashMap::default(), } } @@ -279,11 +278,6 @@ impl<'f> PerFunctionContext<'f> { if let Some(context) = self.load_results.get_mut(allocation) { context.uses -= 1; } - if let Some(store_uses) = self.store_count.get_mut(allocation) { - if *store_uses != 0 { - *store_uses -= 1; - } - } } } } @@ -300,17 +294,6 @@ impl<'f> PerFunctionContext<'f> { context.uses += 1; context.instructions_using_result.push((instruction, block_id)); } - if let Some(store_uses) = self.store_count.get_mut(&value) { - match self.inserter.function.dfg[instruction] { - // We should never expect an allocate but we check it here anyway - Instruction::Allocate => {} - // We are pre-processing reference counts before analyzing an instruction so we should increase - // on any usage of a store address, including the store itself. If a store is removed, we will decrement its count. - _ => { - *store_uses += 1; - } - } - } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { @@ -355,11 +338,6 @@ impl<'f> PerFunctionContext<'f> { if let Some(value) = references.get_known_value(address) { self.inserter.map_value(result, value); self.instructions_to_remove.insert(instruction); - if let Some(store_uses) = self.store_count.get_mut(&address) { - if *store_uses != 0 { - *store_uses -= 1; - } - } } else { references.mark_value_used(address, self.inserter.function); @@ -422,16 +400,6 @@ impl<'f> PerFunctionContext<'f> { context.uses -= 1; } } - if let Some(store_uses) = self.store_count.get_mut(&value) { - if *store_uses != 0 { - *store_uses -= 1; - } - } - if let Some(store_uses) = self.store_count.get_mut(&address) { - if *store_uses != 0 { - *store_uses -= 1; - } - } } let known_value = references.get_known_value(value); @@ -441,41 +409,31 @@ impl<'f> PerFunctionContext<'f> { // TODO: move all of this into a utility method for when removing an instruction // the utility method should have a seen values state to guarantee we do not decrement more than needed self.instructions_to_remove.insert(instruction); - // if let Some(context) = self.load_results.get_mut(&address) { - // if context.uses > 0 { - // context.uses -= 1; - // } - // } - // TODO: This is causing breakages in card game contracts - // if let Some(context) = self.load_results.get_mut(&value) { - // if context.uses > 0 { - // context.uses -= 1; - // } - // } - // value == address so we should only reduce the use once here - if let Some(store_uses) = self.store_count.get_mut(&address) { - if *store_uses != 0 { - *store_uses -= 1; + if let Some(context) = self.load_results.get_mut(&address) { + if context.uses > 0 { + context.uses -= 1; + } + } + if let Some(context) = self.load_results.get_mut(&value) { + if context.uses > 0 { + context.uses -= 1; } } } else { references.last_stores.insert(address, instruction); - - let num_stores = self.store_count.get(&address).copied().unwrap_or(1); - self.store_count.insert(address, num_stores); } } else { references.last_stores.insert(address, instruction); - - let num_stores = self.store_count.get(&address).copied().unwrap_or(1); - self.store_count.insert(address, num_stores); } 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.calls_reference_input.insert(alias); + self.aliased_references + .entry(alias) + .or_default() + .insert(instruction); }); } } @@ -549,23 +507,6 @@ impl<'f> PerFunctionContext<'f> { } } - fn recursively_add_call_arg(&mut self, arg: ValueId, references: &mut Block) { - let arg = self.inserter.function.dfg.resolve(arg); - let aliases = references.get_aliases_for_value(arg); - aliases.for_each(|alias| { - self.calls_reference_input.insert(alias); - }); - if let Some((elements, _)) = self.inserter.function.dfg.get_array_constant(arg) { - for value in elements.iter() { - self.recursively_add_call_arg(*value, references); - } - let aliases = references.collect_all_aliases(elements); - aliases.for_each(|alias| { - self.calls_reference_input.insert(alias); - }); - } - } - fn check_array_aliasing(&self, references: &mut Block, array: ValueId) { if let Some((elements, typ)) = self.inserter.function.dfg.get_array_constant(array) { if Self::contains_references(&typ) { @@ -636,9 +577,6 @@ impl<'f> PerFunctionContext<'f> { context.uses += 1; context.terminators_using_result.push((terminator.clone(), block_id)); } - if let Some(store_uses) = self.store_count.get_mut(&value) { - *store_uses += 1; - } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { @@ -721,7 +659,6 @@ impl<'f> PerFunctionContext<'f> { self.remove_unloaded_last_stores(&removed_loads, &all_terminator_values); self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); - // println!("{}", self.inserter.function); // After possibly removing some instructions we need to map all the instructions // TODO: Add a check whether any loads were actually removed where we actually need to map values let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); @@ -742,7 +679,7 @@ impl<'f> PerFunctionContext<'f> { /// Returns a map of the removed load address to the number of load instructions removed for that address fn remove_unused_loads(&mut self) -> HashMap { let mut removed_loads = HashMap::default(); - for (result, PerFuncLoadResultContext { uses, load_instruction, block_id, .. }) in + for (_, PerFuncLoadResultContext { uses, load_instruction, block_id, .. }) in self.load_results.iter() { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] @@ -751,16 +688,6 @@ impl<'f> PerFunctionContext<'f> { }; // If the load result's counter is equal to zero we can safely remove that load instruction. if *uses == 0 { - if let Some(store_uses) = self.store_count.get_mut(&address) { - if *store_uses != 0 { - *store_uses -= 1; - } - } - if let Some(store_uses) = self.store_count.get_mut(result) { - if *store_uses != 0 { - *store_uses -= 1; - } - } self.return_block_load_locations.remove(&(address, *block_id)); removed_loads.entry(address).and_modify(|counter| *counter += 1).or_insert(1); @@ -808,34 +735,15 @@ impl<'f> PerFunctionContext<'f> { let mut remaining_last_stores: HashMap = HashMap::default(); for (block_id, block) in self.blocks.iter() { for (store_address, store_instruction) in block.last_stores.iter() { - let Instruction::Store { value, .. } = - self.inserter.function.dfg[*store_instruction] - else { - panic!("Should have Store instruction"); - }; - if self.instructions_to_remove.contains(store_instruction) { - // println!("already set to remove"); continue; } - let is_used_in_terminator = all_terminator_values.contains(store_address); - let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); // Determine whether any loads that reference this store address // have been removed while cleaning up unused loads. let loads_removed = if is_return { - // Determine whether the last store is used in the return value - let mut is_return_value = false; - terminator.for_each_value(|return_value| { - self.recursively_check_address_in_terminator( - return_value, - *store_address, - &mut is_return_value, - ); - }); - // If we are in a return terminator, and the last loads of a reference // come before a store to that reference, we can safely remove that store. let store_after_load = if let Some(max_load_index) = @@ -852,11 +760,9 @@ impl<'f> PerFunctionContext<'f> { true }; store_after_load - } else if let (Some(context), Some(loads_removed_counter), Some(store_uses)) = ( - self.last_loads.get(store_address), - removed_loads.get(store_address), - self.store_count.get(store_address), - ) { + } else if let (Some(context), Some(loads_removed_counter)) = + (self.last_loads.get(store_address), removed_loads.get(store_address)) + { // `last_loads` contains the total number of loads for a given load address // If the number of removed loads for a given address is equal to the total number of loads for that address, // we know we can safely remove any stores to that load address. @@ -899,21 +805,22 @@ impl<'f> PerFunctionContext<'f> { if allocation_aliases_parameter == Some(true) { store_alias_used = 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) { + store_alias_used = true; + } } } if loads_removed && !store_alias_used { self.instructions_to_remove.insert(*store_instruction); - if let Some(store_uses) = self.store_count.get_mut(store_address) { - if *store_uses != 0 { - *store_uses -= 1; - } - } - if let Some(store_uses) = self.store_count.get_mut(&value) { - if *store_uses != 0 { - *store_uses -= 1; - } - } if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; } @@ -961,7 +868,7 @@ impl<'f> PerFunctionContext<'f> { if *store_counter != 0 { continue; } - // println!("remaining last stores: store {value} at {store_address}"); + self.instructions_to_remove.insert(*store_instruction); // Map any remaining load results to the value from the removed store @@ -1412,7 +1319,6 @@ mod tests { builder.terminate_with_jmp(b1, vec![plus_one]); let ssa = builder.finish(); - println!("{}", ssa); // Expected result: // acir(inline) fn main f0 { @@ -1433,7 +1339,6 @@ mod tests { // return Field 1 // } let ssa = ssa.mem2reg(); - println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 4); @@ -1531,7 +1436,6 @@ mod tests { // We expect the same result as above. let ssa = ssa.mem2reg(); - println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 4); @@ -1635,7 +1539,6 @@ mod tests { builder.terminate_with_return(vec![return_array]); let ssa = builder.finish(); - println!("{}", ssa); // Expected result: // acir(inline) fn main f0 { @@ -1655,7 +1558,6 @@ mod tests { // return [v33] // } let ssa = ssa.mem2reg(); - println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 3); @@ -1674,7 +1576,7 @@ mod tests { } #[test] - fn keep_unused_store_used_as_alias_across_blocks() { + fn keep_unused_store_only_used_as_an_alias_across_blocks() { // acir(inline) fn main f0 { // b0(v0: u32): // v1 = allocate @@ -1767,7 +1669,6 @@ mod tests { // We expect the same result as above. let ssa = ssa.mem2reg(); - println!("{}", ssa); let main = ssa.main(); // We expect all the stores to remain. From 9cd7ea6853dd5ce202c81fdab790cf70dea1017d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 16 Sep 2024 22:46:21 +0000 Subject: [PATCH 43/58] some cleanup --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 248 +++++++++--------- 1 file changed, 123 insertions(+), 125 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index da11fc4250e..5dea4f748a1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -284,20 +284,14 @@ impl<'f> PerFunctionContext<'f> { } } - fn count_increase_instruction( - &mut self, - value: ValueId, - instruction: InstructionId, - block_id: BasicBlockId, - ) { + fn increase_load_ref_counts(&mut self, value: ValueId) { if let Some(context) = self.load_results.get_mut(&value) { context.uses += 1; - context.instructions_using_result.push((instruction, block_id)); } let array_const = self.inserter.function.dfg.get_array_constant(value); if let Some((values, _)) = array_const { for array_value in values { - self.count_increase_instruction(array_value, instruction, block_id); + self.increase_load_ref_counts(array_value); } } } @@ -324,7 +318,7 @@ impl<'f> PerFunctionContext<'f> { }); for value in collect_values { - self.count_increase_instruction(value, instruction, block_id); + self.increase_load_ref_counts(value); } match &self.inserter.function.dfg[instruction] { @@ -388,17 +382,13 @@ impl<'f> PerFunctionContext<'f> { let Instruction::Store { address, value } = self.inserter.function.dfg[*last_store] else { - panic!("Must have a Store instruction here"); + panic!("Should have a store instruction here"); }; - if let Some(context) = self.load_results.get_mut(&value) { - if context.uses > 0 { - context.uses -= 1; - } - } if let Some(context) = self.load_results.get_mut(&address) { - if context.uses > 0 { - context.uses -= 1; - } + context.uses -= 1; + } + if let Some(context) = self.load_results.get_mut(&value) { + context.uses -= 1; } } @@ -406,18 +396,12 @@ impl<'f> PerFunctionContext<'f> { if let Some(known_value) = known_value { let known_value_is_address = known_value == address; if known_value_is_address { - // TODO: move all of this into a utility method for when removing an instruction - // the utility method should have a seen values state to guarantee we do not decrement more than needed self.instructions_to_remove.insert(instruction); if let Some(context) = self.load_results.get_mut(&address) { - if context.uses > 0 { - context.uses -= 1; - } + context.uses -= 1; } if let Some(context) = self.load_results.get_mut(&value) { - if context.uses > 0 { - context.uses -= 1; - } + context.uses -= 1; } } else { references.last_stores.insert(address, instruction); @@ -567,24 +551,6 @@ impl<'f> PerFunctionContext<'f> { self.inserter.function.dfg.data_bus = databus.map_values(|t| self.inserter.resolve(t)); } - fn terminator_count_increase( - &mut self, - value: ValueId, - terminator: &TerminatorInstruction, - block_id: BasicBlockId, - ) { - if let Some(context) = self.load_results.get_mut(&value) { - context.uses += 1; - context.terminators_using_result.push((terminator.clone(), block_id)); - } - let array_const = self.inserter.function.dfg.get_array_constant(value); - if let Some((values, _)) = array_const { - for array_value in values { - self.terminator_count_increase(array_value, terminator, block_id); - } - } - } - fn handle_terminator(&mut self, block: BasicBlockId, references: &mut Block) { self.inserter.map_terminator_in_place(block); @@ -598,7 +564,7 @@ impl<'f> PerFunctionContext<'f> { let terminator = terminator.clone(); for value in collect_values.iter() { - self.terminator_count_increase(*value, &terminator, block); + self.increase_load_ref_counts(*value); } match &terminator { @@ -730,7 +696,6 @@ impl<'f> PerFunctionContext<'f> { let block_params = self.inserter.function.dfg.block_parameters(*block_id); per_func_block_params.extend(block_params.iter()); } - let func_params = self.inserter.function.parameters(); let mut remaining_last_stores: HashMap = HashMap::default(); for (block_id, block) in self.blocks.iter() { @@ -739,87 +704,21 @@ impl<'f> PerFunctionContext<'f> { continue; } - let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); - let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); - // Determine whether any loads that reference this store address - // have been removed while cleaning up unused loads. - let loads_removed = if is_return { - // If we are in a return terminator, and the last loads of a reference - // come before a store to that reference, we can safely remove that store. - let store_after_load = if let Some(max_load_index) = - self.return_block_load_locations.get(&(*store_address, *block_id)) - { - let store_index = self.inserter.function.dfg[*block_id] - .instructions() - .iter() - .position(|id| *id == *store_instruction) - .expect("Store instruction should exist in the return block"); - store_index > *max_load_index - } else { - // Otherwise there is no load in this block - true - }; - store_after_load - } else if let (Some(context), Some(loads_removed_counter)) = - (self.last_loads.get(store_address), removed_loads.get(store_address)) - { - // `last_loads` contains the total number of loads for a given load address - // If the number of removed loads for a given address is equal to the total number of loads for that address, - // we know we can safely remove any stores to that load address. - context.num_loads == *loads_removed_counter - } else { - self.last_loads.get(store_address).is_none() - }; - - // 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. - let reference_parameters = func_params - .iter() - .filter(|param| self.inserter.function.dfg.value_is_reference(**param)) - .collect::>(); - - let mut store_alias_used = false; - 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) { - store_alias_used = true; - } - - let allocation_aliases_parameter = - aliases.any(|alias| per_func_block_params.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - store_alias_used = true; - } - - let allocation_aliases_parameter = - aliases.any(|alias| self.calls_reference_input.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - store_alias_used = true; - } - - let allocation_aliases_parameter = - aliases.any(|alias| all_terminator_values.contains(&alias)); - if allocation_aliases_parameter == Some(true) { - store_alias_used = true; - } + let all_loads_removed = self.all_loads_removed_for_address( + store_address, + *store_instruction, + *block_id, + removed_loads, + ); - 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) { - store_alias_used = true; - } - } - } + let store_alias_used = self.is_store_alias_used( + store_address, + block, + all_terminator_values, + &per_func_block_params, + ); - if loads_removed && !store_alias_used { + if all_loads_removed && !store_alias_used { self.instructions_to_remove.insert(*store_instruction); if let Some((_, counter)) = remaining_last_stores.get_mut(store_address) { *counter -= 1; @@ -834,6 +733,105 @@ impl<'f> PerFunctionContext<'f> { remaining_last_stores } + fn all_loads_removed_for_address( + &self, + store_address: &ValueId, + store_instruction: InstructionId, + block_id: BasicBlockId, + removed_loads: &HashMap, + ) -> bool { + let terminator = self.inserter.function.dfg[block_id].unwrap_terminator(); + let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); + // Determine whether any loads that reference this store address + // have been removed while cleaning up unused loads. + if is_return { + // If we are in a return terminator, and the last loads of a reference + // come before a store to that reference, we can safely remove that store. + let store_after_load = if let Some(max_load_index) = + self.return_block_load_locations.get(&(*store_address, block_id)) + { + let store_index = self.inserter.function.dfg[block_id] + .instructions() + .iter() + .position(|id| *id == store_instruction) + .expect("Store instruction should exist in the return block"); + store_index > *max_load_index + } else { + // Otherwise there is no load in this block + true + }; + store_after_load + } else if let (Some(context), Some(loads_removed_counter)) = + (self.last_loads.get(store_address), removed_loads.get(store_address)) + { + // `last_loads` contains the total number of loads for a given load address + // If the number of removed loads for a given address is equal to the total number of loads for that address, + // we know we can safely remove any stores to that load address. + context.num_loads == *loads_removed_counter + } else { + self.last_loads.get(store_address).is_none() + } + } + + // 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 func_params = self.inserter.function.parameters(); + let reference_parameters = func_params + .iter() + .filter(|param| self.inserter.function.dfg.value_is_reference(**param)) + .collect::>(); + + let mut store_alias_used = false; + 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) { + store_alias_used = true; + } + + let allocation_aliases_parameter = + aliases.any(|alias| per_func_block_params.contains(&alias)); + if allocation_aliases_parameter == Some(true) { + store_alias_used = true; + } + + let allocation_aliases_parameter = + aliases.any(|alias| self.calls_reference_input.contains(&alias)); + if allocation_aliases_parameter == Some(true) { + store_alias_used = true; + } + + let allocation_aliases_parameter = + aliases.any(|alias| all_terminator_values.contains(&alias)); + if allocation_aliases_parameter == Some(true) { + store_alias_used = 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) { + store_alias_used = true; + } + } + } + + store_alias_used + } + /// Check if any remaining last stores are only used in a single load fn remove_remaining_last_stores( &mut self, From b6750e8a70662fd2282dc3a9881b1b9fd1366aee Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 16 Sep 2024 22:47:22 +0000 Subject: [PATCH 44/58] remove debugging stuff --- compiler/noirc_driver/src/lib.rs | 2 +- tooling/nargo/src/ops/compile.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 8004a77bd14..2e43dfcb527 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -421,7 +421,7 @@ fn compile_contract_inner( let mut functions = Vec::new(); let mut errors = Vec::new(); let mut warnings = Vec::new(); - println!("contract {}", contract.name.clone()); + for contract_function in &contract.functions { let function_id = contract_function.function_id; let is_entry_point = contract_function.is_entry_point; diff --git a/tooling/nargo/src/ops/compile.rs b/tooling/nargo/src/ops/compile.rs index 81401f66f05..cd9ccf67957 100644 --- a/tooling/nargo/src/ops/compile.rs +++ b/tooling/nargo/src/ops/compile.rs @@ -36,7 +36,7 @@ pub fn compile_workspace( }) .collect(); let contract_results: Vec> = contract_packages - .iter() + .par_iter() .map(|package| compile_contract(file_manager, parsed_files, package, compile_options)) .collect(); From 5997cdda3e72219a476e47d0f0e200f06393c4b8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 16 Sep 2024 23:16:20 +0000 Subject: [PATCH 45/58] block references test in the debugger --- tooling/debugger/ignored-tests.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 745971d9b28..87a9b2b3401 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -3,4 +3,5 @@ debug_logs is_unconstrained macros references -regression_4709 \ No newline at end of file +regression_4709 +references_testing \ No newline at end of file From b2a435aa92faa69ae8d0056ebf924a7a3498a509 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 14:05:53 +0000 Subject: [PATCH 46/58] some cleanup --- compiler/noirc_driver/src/lib.rs | 1 - .../noirc_evaluator/src/ssa/function_builder/mod.rs | 2 +- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 12 +----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 2e43dfcb527..18a13517b75 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -421,7 +421,6 @@ fn compile_contract_inner( let mut functions = Vec::new(); let mut errors = Vec::new(); let mut warnings = Vec::new(); - for contract_function in &contract.functions { let function_id = contract_function.function_id; let is_entry_point = contract_function.is_entry_point; diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 06e5e18dda5..04d4e893bf8 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -235,7 +235,7 @@ impl FunctionBuilder { if operator != BinaryOp::Shl && operator != BinaryOp::Shr { assert_eq!( lhs_type, rhs_type, - "ICE - Binary instruction operands must have the same type {lhs} and {rhs}, {operator}" + "ICE - Binary instruction operands must have the same type" ); } let instruction = Instruction::Binary(Binary { lhs, rhs, operator }); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 5dea4f748a1..723f4b874f2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -159,21 +159,11 @@ struct PerFuncLoadResultContext { load_instruction: InstructionId, /// Block of the load instruction that produced a given result block_id: BasicBlockId, - /// Instructions that use a given load result - instructions_using_result: Vec<(InstructionId, BasicBlockId)>, - /// Terminators that use a given load result - terminators_using_result: Vec<(TerminatorInstruction, BasicBlockId)>, } impl PerFuncLoadResultContext { fn new(load_instruction: InstructionId, block_id: BasicBlockId) -> Self { - Self { - uses: 0, - load_instruction, - block_id, - instructions_using_result: vec![], - terminators_using_result: vec![], - } + Self { uses: 0, load_instruction, block_id } } } From e107d3307e4b9b528397aa17fa222741efb52617 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 16:31:26 +0000 Subject: [PATCH 47/58] move terminator collection to remove_unloaded_last_stores and add stores_were_removed flag --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 723f4b874f2..0421a8f1b18 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -600,33 +600,27 @@ impl<'f> PerFunctionContext<'f> { /// We collect state about any final loads and stores to a given address during the initial mem2reg pass. /// We can then utilize this state to clean up any loads and stores that may have been missed. fn cleanup_function(&mut self) { - let mut all_terminator_values = HashSet::default(); - for (block_id, _) in self.blocks.iter() { - let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); - terminator.for_each_value(|value| { - self.recursively_add_values(value, &mut all_terminator_values); - }); - } - // Removing remaining unused loads during mem2reg can help expose removable stores that the initial // mem2reg pass deemed we could not remove due to the existence of those unused loads. let removed_loads = self.remove_unused_loads(); - let remaining_last_stores = - self.remove_unloaded_last_stores(&removed_loads, &all_terminator_values); - self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); - - // After possibly removing some instructions we need to map all the instructions - // TODO: Add a check whether any loads were actually removed where we actually need to map values - let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); - block_order.reverse(); - for block in block_order { - let instructions = self.inserter.function.dfg[block].take_instructions(); - for instruction in instructions { - if !self.instructions_to_remove.contains(&instruction) { - self.inserter.push_instruction(instruction, block); + let remaining_last_stores = self.remove_unloaded_last_stores(&removed_loads); + let stores_were_removed = + self.remove_remaining_last_stores(&removed_loads, &remaining_last_stores); + + // When removing some last loads with the last stores we will map the load result to the store value. + // We need to then map all the instructions again as we do not know which instructions are reliant on the load result. + if stores_were_removed { + let mut block_order = PostOrder::with_function(self.inserter.function).into_vec(); + block_order.reverse(); + for block in block_order { + let instructions = self.inserter.function.dfg[block].take_instructions(); + for instruction in instructions { + if !self.instructions_to_remove.contains(&instruction) { + self.inserter.push_instruction(instruction, block); + } } + self.inserter.map_terminator_in_place(block); } - self.inserter.map_terminator_in_place(block); } } @@ -679,12 +673,17 @@ impl<'f> PerFunctionContext<'f> { fn remove_unloaded_last_stores( &mut self, removed_loads: &HashMap, - all_terminator_values: &HashSet, ) -> HashMap { + let mut all_terminator_values = HashSet::default(); let mut per_func_block_params: HashSet = HashSet::default(); for (block_id, _) in self.blocks.iter() { let block_params = self.inserter.function.dfg.block_parameters(*block_id); per_func_block_params.extend(block_params.iter()); + + let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); + terminator.for_each_value(|value| { + self.recursively_add_values(value, &mut all_terminator_values); + }); } let mut remaining_last_stores: HashMap = HashMap::default(); @@ -704,7 +703,7 @@ impl<'f> PerFunctionContext<'f> { let store_alias_used = self.is_store_alias_used( store_address, block, - all_terminator_values, + &all_terminator_values, &per_func_block_params, ); @@ -823,11 +822,13 @@ impl<'f> PerFunctionContext<'f> { } /// Check if any remaining last stores are only used in a single load + /// Returns true if any stores were removed. fn remove_remaining_last_stores( &mut self, removed_loads: &HashMap, remaining_last_stores: &HashMap, - ) { + ) -> bool { + let mut stores_were_removed = false; // Filter out any still in use load results and any load results that do not contain addresses from the remaining last stores self.load_results.retain(|_, PerFuncLoadResultContext { load_instruction, uses, .. }| { let Instruction::Load { address } = self.inserter.function.dfg[*load_instruction] @@ -875,8 +876,11 @@ impl<'f> PerFunctionContext<'f> { // as we do not know what instructions depend upon this result self.inserter.map_value(*result, value); self.instructions_to_remove.insert(context.load_instruction); + + stores_were_removed = true; } } + stores_were_removed } } From af0a3c8f4a4d6dee88330865a410cd61c9260b85 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 17:44:36 +0000 Subject: [PATCH 48/58] add doc comments --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 0421a8f1b18..8dc3bd19b63 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -18,6 +18,35 @@ //! - 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. +//! - The steps above on their own can sometimes miss known references. +//! This most commonly occurs in the case of loops, where we may have allocations preceding a loop that are known, +//! but the loop body's blocks are predecessors to the loop header block, causing those known allocations to be marked unknown. +//! In certain cases we may be able to remove these allocations that precede a loop. +//! For example, if a reference is not stored to again in the loop we should be able to remove that store which precedes the loop. +//! - To handle cases such as the one laid out above, we maintain state per function. +//! The state contains the following: +//! - For each load address we store the number of loads from a given address, +//! the last load instruction from a given load across all blocks, and the respective block id of that instruction. +//! - A mapping of each load result to its number of uses, the load instruction that produced the given result, and the respective block id of that instruction. +//! - A set of the references passed into a call of another function entry point. +//! - Maps the references which have been aliased to the instructions that aliased that reference. +//! - As we go through each instruction, if a load result has been used we increment its usage counter. +//! Upon removing an instruction, we decrement the load result counter. +//! After analyzing all of a function's blocks we can analyze the per function state: +//! - If we find that a load result is unused we remove that load. +//! - We can then remove a store if the following conditions are met: +//! - All loads to a given address have been removed +//! - None of the aliases of a reference are used in any of the following: +//! - Block parameters, function parameters, call arguments, terminator arguments +//! - The store address also should not be aliased +//! - If a store is in a return block, we can have special handling that only checks if there is a load after +//! that store in the return block. In the case of a return block, even if there are other loads +//! in preceding blocks we can safely remove those stores. +//! - To further catch any stores to references which are never loaded, we can count the number of stores +//! that were removed in the previous step. If there is only a single store leftover, we can safely map +//! the value of this final store to any loads of that store. +//! +//! - Throughout the pass we also collect information about loads and stores //! //! From there, to figure out the value of each reference at the end of block, iterate each instruction: //! - On `Instruction::Allocate`: From 97db5343a65c41bf061e7ee0b62510067ac16648 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 17:47:01 +0000 Subject: [PATCH 49/58] rename references_testing to reference_only_used_as_alias --- .../Nargo.toml | 0 .../Prover.toml | 0 .../src/main.nr | 2 ++ 3 files changed, 2 insertions(+) rename test_programs/execution_success/{references_testing => reference_only_used_as_alias}/Nargo.toml (100%) rename test_programs/execution_success/{references_testing => reference_only_used_as_alias}/Prover.toml (100%) rename test_programs/execution_success/{references_testing => reference_only_used_as_alias}/src/main.nr (91%) diff --git a/test_programs/execution_success/references_testing/Nargo.toml b/test_programs/execution_success/reference_only_used_as_alias/Nargo.toml similarity index 100% rename from test_programs/execution_success/references_testing/Nargo.toml rename to test_programs/execution_success/reference_only_used_as_alias/Nargo.toml diff --git a/test_programs/execution_success/references_testing/Prover.toml b/test_programs/execution_success/reference_only_used_as_alias/Prover.toml similarity index 100% rename from test_programs/execution_success/references_testing/Prover.toml rename to test_programs/execution_success/reference_only_used_as_alias/Prover.toml diff --git a/test_programs/execution_success/references_testing/src/main.nr b/test_programs/execution_success/reference_only_used_as_alias/src/main.nr similarity index 91% rename from test_programs/execution_success/references_testing/src/main.nr rename to test_programs/execution_success/reference_only_used_as_alias/src/main.nr index c740bdff662..c04d68d1748 100644 --- a/test_programs/execution_success/references_testing/src/main.nr +++ b/test_programs/execution_success/reference_only_used_as_alias/src/main.nr @@ -76,6 +76,8 @@ unconstrained fn func(input: u32) { assert(**ref == 2); } +// This test aims to allocate a reference which is aliased and only accessed through its alias +// across multiple blocks. We want to guarantee that this allocation is not removed. fn main(input: [Field; 4], randomness: Field, context_input: u32) { let b = [context_input, context_input, context_input]; let mut context = Context { a: context_input, b, log_hashes: BoundedVec::new() }; From 447947a9a1eb5fc46870d45c5684c1b85004a5ca Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 17:48:03 +0000 Subject: [PATCH 50/58] cleanup comment --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 8dc3bd19b63..66c3f7eea0c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -45,8 +45,6 @@ //! - To further catch any stores to references which are never loaded, we can count the number of stores //! that were removed in the previous step. If there is only a single store leftover, we can safely map //! the value of this final store to any loads of that store. -//! -//! - Throughout the pass we also collect information about loads and stores //! //! From there, to figure out the value of each reference at the end of block, iterate each instruction: //! - On `Instruction::Allocate`: From ea858a1838b84f9ec0c60df90b25d44cfb26b833 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 17:56:09 +0000 Subject: [PATCH 51/58] move doc comments location --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 59 ++++++++++--------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 66c3f7eea0c..347b09fab27 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -18,33 +18,6 @@ //! - 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. -//! - The steps above on their own can sometimes miss known references. -//! This most commonly occurs in the case of loops, where we may have allocations preceding a loop that are known, -//! but the loop body's blocks are predecessors to the loop header block, causing those known allocations to be marked unknown. -//! In certain cases we may be able to remove these allocations that precede a loop. -//! For example, if a reference is not stored to again in the loop we should be able to remove that store which precedes the loop. -//! - To handle cases such as the one laid out above, we maintain state per function. -//! The state contains the following: -//! - For each load address we store the number of loads from a given address, -//! the last load instruction from a given load across all blocks, and the respective block id of that instruction. -//! - A mapping of each load result to its number of uses, the load instruction that produced the given result, and the respective block id of that instruction. -//! - A set of the references passed into a call of another function entry point. -//! - Maps the references which have been aliased to the instructions that aliased that reference. -//! - As we go through each instruction, if a load result has been used we increment its usage counter. -//! Upon removing an instruction, we decrement the load result counter. -//! After analyzing all of a function's blocks we can analyze the per function state: -//! - If we find that a load result is unused we remove that load. -//! - We can then remove a store if the following conditions are met: -//! - All loads to a given address have been removed -//! - None of the aliases of a reference are used in any of the following: -//! - Block parameters, function parameters, call arguments, terminator arguments -//! - The store address also should not be aliased -//! - If a store is in a return block, we can have special handling that only checks if there is a load after -//! that store in the return block. In the case of a return block, even if there are other loads -//! in preceding blocks we can safely remove those stores. -//! - To further catch any stores to references which are never loaded, we can count the number of stores -//! that were removed in the previous step. If there is only a single store leftover, we can safely map -//! the value of this final store to any loads of that store. //! //! From there, to figure out the value of each reference at the end of block, iterate each instruction: //! - On `Instruction::Allocate`: @@ -88,6 +61,38 @@ //! 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 loop indices. +//! +//! As stated above, the algorithm above can sometimes miss known references. +//! This most commonly occurs in the case of loops, where we may have allocations preceding a loop that are known, +//! but the loop body's blocks are predecessors to the loop header block, causing those known allocations to be marked unknown. +//! In certain cases we may be able to remove these allocations that precede a loop. +//! For example, if a reference is not stored to again in the loop we should be able to remove that store which precedes the loop. +//! +//! To handle cases such as the one laid out above, we maintain some extra state per function, +//! that we will analyze after the initial run through of all the blocks. +//! We refer to this as the "function cleanup" and it requires having already iterated through all blocks. +//! +//! The state contains the following: +//! - For each load address we store the number of loads from a given address, +//! the last load instruction from a given load across all blocks, and the respective block id of that instruction. +//! - A mapping of each load result to its number of uses, the load instruction that produced the given result, and the respective block id of that instruction. +//! - A set of the references passed into a call of another function entry point. +//! - Maps the references which have been aliased to the instructions that aliased that reference. +//! - As we go through each instruction, if a load result has been used we increment its usage counter. +//! Upon removing an instruction, we decrement the load result counter. +//! After analyzing all of a function's blocks we can analyze the per function state: +//! - If we find that a load result is unused we remove that load. +//! - We can then remove a store if the following conditions are met: +//! - All loads to a given address have been removed +//! - None of the aliases of a reference are used in any of the following: +//! - Block parameters, function parameters, call arguments, terminator arguments +//! - The store address also should not be aliased +//! - If a store is in a return block, we can have special handling that only checks if there is a load after +//! that store in the return block. In the case of a return block, even if there are other loads +//! in preceding blocks we can safely remove those stores. +//! - To further catch any stores to references which are never loaded, we can count the number of stores +//! that were removed in the previous step. If there is only a single store leftover, we can safely map +//! the value of this final store to any loads of that store. mod alias_set; mod block; From 2ef1d4155727ac32b2f0d6ece6a63aa70e613526 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 17:59:51 +0000 Subject: [PATCH 52/58] typo --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 347b09fab27..9f3402a3d7e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -69,7 +69,7 @@ //! For example, if a reference is not stored to again in the loop we should be able to remove that store which precedes the loop. //! //! To handle cases such as the one laid out above, we maintain some extra state per function, -//! that we will analyze after the initial run through of all the blocks. +//! that we will analyze after the initial run through all of the blocks. //! We refer to this as the "function cleanup" and it requires having already iterated through all blocks. //! //! The state contains the following: From c760ace2609e39aaaf086a9a81c5493cf1f1ec44 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 18:00:24 +0000 Subject: [PATCH 53/58] clarity improvement --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 9f3402a3d7e..15b0c5dc3c5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -74,7 +74,7 @@ //! //! The state contains the following: //! - For each load address we store the number of loads from a given address, -//! the last load instruction from a given load across all blocks, and the respective block id of that instruction. +//! the last load instruction from a given address across all blocks, and the respective block id of that instruction. //! - A mapping of each load result to its number of uses, the load instruction that produced the given result, and the respective block id of that instruction. //! - A set of the references passed into a call of another function entry point. //! - Maps the references which have been aliased to the instructions that aliased that reference. From b137682b8dad7edf0e8df2eedbee39272e644a75 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 18:01:25 +0000 Subject: [PATCH 54/58] one more comment improvement --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 15b0c5dc3c5..e4de4fb6911 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -76,7 +76,7 @@ //! - For each load address we store the number of loads from a given address, //! the last load instruction from a given address across all blocks, and the respective block id of that instruction. //! - A mapping of each load result to its number of uses, the load instruction that produced the given result, and the respective block id of that instruction. -//! - A set of the references passed into a call of another function entry point. +//! - A set of the references and their aliases passed as an argument to a call. //! - Maps the references which have been aliased to the instructions that aliased that reference. //! - As we go through each instruction, if a load result has been used we increment its usage counter. //! Upon removing an instruction, we decrement the load result counter. From c73a25d4db0309bb7afefbe24c1e70b27df5cee0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 18:02:12 +0000 Subject: [PATCH 55/58] reference counter more explicitly --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index e4de4fb6911..8a42628dd3d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -81,7 +81,7 @@ //! - As we go through each instruction, if a load result has been used we increment its usage counter. //! Upon removing an instruction, we decrement the load result counter. //! After analyzing all of a function's blocks we can analyze the per function state: -//! - If we find that a load result is unused we remove that load. +//! - If we find that a load result's usage counter equals zero, we can remove that load. //! - We can then remove a store if the following conditions are met: //! - All loads to a given address have been removed //! - None of the aliases of a reference are used in any of the following: From fb18d7e2a29fcda74788b93754efd5e12df3feaf Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 18:02:45 +0000 Subject: [PATCH 56/58] one more clearer sentence --- compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 8a42628dd3d..9c60be37bc9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -86,7 +86,7 @@ //! - All loads to a given address have been removed //! - None of the aliases of a reference are used in any of the following: //! - Block parameters, function parameters, call arguments, terminator arguments -//! - The store address also should not be aliased +//! - The store address is not aliased. //! - If a store is in a return block, we can have special handling that only checks if there is a load after //! that store in the return block. In the case of a return block, even if there are other loads //! in preceding blocks we can safely remove those stores. From 06c99e6806524a4f81a9187f1c5dfacb45ca9cc8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 18:16:08 +0000 Subject: [PATCH 57/58] put renamed test in debugger ignored tests --- tooling/debugger/ignored-tests.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/debugger/ignored-tests.txt b/tooling/debugger/ignored-tests.txt index 87a9b2b3401..78e14397938 100644 --- a/tooling/debugger/ignored-tests.txt +++ b/tooling/debugger/ignored-tests.txt @@ -4,4 +4,4 @@ is_unconstrained macros references regression_4709 -references_testing \ No newline at end of file +reference_only_used_as_alias \ No newline at end of file From 30a3d8c208b9fbb078e31a9e8756a3cc24fa50f0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Sep 2024 18:35:30 +0000 Subject: [PATCH 58/58] missd nargo toml save --- .../execution_success/reference_only_used_as_alias/Nargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/execution_success/reference_only_used_as_alias/Nargo.toml b/test_programs/execution_success/reference_only_used_as_alias/Nargo.toml index ad0781cbd30..d7531756822 100644 --- a/test_programs/execution_success/reference_only_used_as_alias/Nargo.toml +++ b/test_programs/execution_success/reference_only_used_as_alias/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "references_testing" +name = "reference_only_used_as_alias" type = "bin" authors = [""] compiler_version = ">=0.33.0"