diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 0c7427dc228..f09357b324c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -14,7 +14,7 @@ use super::{ }, map::DenseMap, types::{NumericType, Type}, - value::{Value, ValueId}, + value::{Value, ValueId, resolve_value}, }; use acvm::{FieldElement, acir::AcirField}; @@ -383,47 +383,40 @@ impl DataFlowGraph { self.instructions[id] = instruction; } - /// Replaces all values in the given blocks with the values in the given map. - /// - /// This method should be preferred over `set_value_from_id` which might eventually be removed. - pub(crate) fn replace_values_in_blocks( + /// Replaces values in the given block terminator (if it has any) according to the given HashMap. + pub(crate) fn replace_values_in_block( &mut self, - blocks: impl Iterator, + block: BasicBlockId, values_to_replace: &HashMap, ) { - if values_to_replace.is_empty() { - return; - } + self.replace_values_in_block_instructions(block, values_to_replace); + self.replace_values_in_block_terminator(block, values_to_replace); + } - let replacement_fn = |value_id| { - if let Some(replacement_id) = values_to_replace.get(&value_id) { - *replacement_id - } else { - value_id - } - }; - - for block in blocks { - // Replace in all the block's instructions - for instruction_id in self.blocks[block].instructions() { - let instruction = &mut self.instructions[*instruction_id]; - instruction.map_values_mut(replacement_fn); - - // Make sure we also replace the instruction results - let results = self.results.get_mut(instruction_id); - if let Some(results) = results { - for result in results { - if let Some(replacement_id) = values_to_replace.get(result) { - *result = *replacement_id; - } - } - } - } + /// Replaces values in the given block instructions according to the given HashMap. + pub(crate) fn replace_values_in_block_instructions( + &mut self, + block: BasicBlockId, + values_to_replace: &HashMap, + ) { + let instruction_ids = self.blocks[block].take_instructions(); + for instruction_id in &instruction_ids { + let instruction = &mut self[*instruction_id]; + instruction.replace_values(&values_to_replace); + } + *self[block].instructions_mut() = instruction_ids; + } - // Finally, the value might show up in a terminator - if self[block].terminator().is_some() { - self[block].unwrap_terminator_mut().map_values_mut(replacement_fn); - } + /// Replaces values in the given block terminator (if it has any) according to the given HashMap. + pub(crate) fn replace_values_in_block_terminator( + &mut self, + block: BasicBlockId, + values_to_replace: &HashMap, + ) { + if self[block].terminator().is_some() { + self[block] + .unwrap_terminator_mut() + .map_values_mut(|value_id| resolve_value(values_to_replace, value_id)); } } @@ -603,30 +596,6 @@ impl DataFlowGraph { matches!(self.values[value].get_type().as_ref(), Type::Reference(_)) } - /// Replaces an instruction result with a fresh id. - pub(crate) fn replace_result( - &mut self, - instruction_id: InstructionId, - prev_value_id: ValueId, - ) -> ValueId { - let typ = self.type_of_value(prev_value_id); - let results = self.results.get_mut(&instruction_id).unwrap(); - let res_position = results - .iter() - .position(|&id| id == prev_value_id) - .expect("Result id not found while replacing"); - - let value_id = self.values.insert(Value::Instruction { - typ, - position: res_position, - instruction: instruction_id, - }); - - // Replace the value in list of results for this instruction - results[res_position] = value_id; - value_id - } - /// Returns all of result values which are attached to this instruction. pub(crate) fn instruction_results(&self, instruction_id: InstructionId) -> &[ValueId] { self.results.get(&instruction_id).expect("expected a list of Values").as_slice() diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index ee4adfe89e2..ce6eb6457fe 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1,3 +1,4 @@ +use fxhash::FxHashMap as HashMap; use std::hash::{Hash, Hasher}; use acvm::acir::{BlackBoxFunc, circuit::ErrorSelector}; @@ -14,7 +15,7 @@ use super::{ dfg::DataFlowGraph, map::Id, types::{NumericType, Type}, - value::{Value, ValueId}, + value::{Value, ValueId, resolve_value}, }; pub(crate) mod binary; @@ -422,6 +423,13 @@ impl Instruction { } } + /// Replaces values present in this instruction with other values according to the given HashMap. + pub(crate) fn replace_values(&mut self, values_to_replace: &HashMap) { + if !values_to_replace.is_empty() { + self.map_values_mut(|value_id| resolve_value(values_to_replace, value_id)); + } + } + /// Maps each ValueId inside this instruction to a new ValueId, returning the new instruction. /// Note that the returned instruction is fresh and will not have an assigned InstructionId /// until it is manually inserted in a DataFlowGraph later. diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index b6e9d987576..f2236964015 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -1,3 +1,4 @@ +use fxhash::FxHashMap as HashMap; use std::borrow::Cow; use acvm::FieldElement; @@ -71,3 +72,14 @@ impl Value { } } } + +pub(super) fn resolve_value( + values_to_replace: &HashMap, + value_id: ValueId, +) -> ValueId { + if let Some(replacement_id) = values_to_replace.get(&value_id) { + resolve_value(values_to_replace, *replacement_id) + } else { + value_id + } +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 837a35126ac..29685ac9f90 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -1,7 +1,7 @@ use crate::ssa::{ ir::{ function::Function, - instruction::{Instruction, InstructionId, Intrinsic}, + instruction::{Instruction, Intrinsic}, types::{NumericType, Type}, value::Value, }, @@ -28,52 +28,45 @@ impl Ssa { impl Function { pub(crate) fn as_slice_optimization(&mut self) { - let known_slice_lengths = known_slice_lengths(self); - replace_known_slice_lengths(self, known_slice_lengths); - } -} + let mut values_to_replace = HashMap::default(); + + for block in self.reachable_blocks() { + let instruction_ids = self.dfg[block].take_instructions(); + for instruction_id in &instruction_ids { + let (target_func, first_argument) = { + let instruction = &mut self.dfg[*instruction_id]; + instruction.replace_values(&values_to_replace); -fn known_slice_lengths(func: &Function) -> HashMap { - let mut known_slice_lengths = HashMap::default(); - for block_id in func.reachable_blocks() { - let block = &func.dfg[block_id]; - for instruction_id in block.instructions() { - let (target_func, arguments) = match &func.dfg[*instruction_id] { - Instruction::Call { func, arguments } => (func, arguments), - _ => continue, - }; + let (target_func, arguments) = match &instruction { + Instruction::Call { func, arguments } => (func, arguments), + _ => continue, + }; - match &func.dfg[*target_func] { - Value::Intrinsic(Intrinsic::AsSlice) => { - let array_typ = func.dfg.type_of_value(arguments[0]); - if let Type::Array(_, length) = array_typ { - known_slice_lengths.insert(*instruction_id, length); - } else { - unreachable!("AsSlice called with non-array {}", array_typ); + (*target_func, arguments.first().copied()) + }; + + match &self.dfg[target_func] { + Value::Intrinsic(Intrinsic::AsSlice) => { + let first_argument = first_argument.unwrap(); + let array_typ = self.dfg.type_of_value(first_argument); + if let Type::Array(_, length) = array_typ { + let call_returns = self.dfg.instruction_results(*instruction_id); + let original_slice_length = call_returns[0]; + let known_length = + self.dfg.make_constant(length.into(), NumericType::length_type()); + values_to_replace.insert(original_slice_length, known_length); + } else { + unreachable!("AsSlice called with non-array {}", array_typ); + } } - } - _ => continue, - }; + _ => continue, + }; + } + + *self.dfg[block].instructions_mut() = instruction_ids; + self.dfg.replace_values_in_block_terminator(block, &values_to_replace); } } - known_slice_lengths -} - -fn replace_known_slice_lengths( - func: &mut Function, - known_slice_lengths: HashMap, -) { - known_slice_lengths.into_iter().for_each(|(instruction_id, known_length)| { - let call_returns = func.dfg.instruction_results(instruction_id); - let original_slice_length = call_returns[0]; - - // We won't use the new id for the original unknown length. - // This isn't strictly necessary as a new result will be defined the next time for which the instruction - // is reinserted but this avoids leaving the program in an invalid state. - func.dfg.replace_result(instruction_id, original_slice_length); - let known_length = func.dfg.make_constant(known_length.into(), NumericType::length_type()); - func.dfg.set_value_from_id(original_slice_length, known_length); - }); } #[cfg(test)] diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_truncate_after_range_check.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_truncate_after_range_check.rs index 9273453d6cb..58316eda686 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_truncate_after_range_check.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_truncate_after_range_check.rs @@ -29,10 +29,15 @@ impl Function { for block in &blocks { let block = *block; let mut instructions_to_remove = HashSet::default(); + let mut instruction_ids = self.dfg[block].take_instructions(); - for instruction_id in self.dfg[block].instructions() { - let instruction = &self.dfg[*instruction_id]; + for instruction_id in &instruction_ids { + if !values_to_replace.is_empty() { + let instruction = &mut self.dfg[*instruction_id]; + instruction.replace_values(&values_to_replace); + } + let instruction = &self.dfg[*instruction_id]; match instruction { // If this is a range_check instruction, associate the max bit size with the value Instruction::RangeCheck { value, max_bit_size, .. } => { @@ -66,16 +71,12 @@ impl Function { } } - if instructions_to_remove.is_empty() { - continue; + if !instructions_to_remove.is_empty() { + instruction_ids.retain(|instruction| !instructions_to_remove.contains(instruction)); } - - self.dfg[block] - .instructions_mut() - .retain(|instruction| !instructions_to_remove.contains(instruction)); + *self.dfg[block].instructions_mut() = instruction_ids; + self.dfg.replace_values_in_block_terminator(block, &values_to_replace); } - - self.dfg.replace_values_in_blocks(blocks.into_iter(), &values_to_replace); } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index b042cf9a41e..b14bfa55d1d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -19,11 +19,13 @@ use crate::ssa::{ cfg::ControlFlowGraph, function::{Function, RuntimeType}, instruction::{Instruction, TerminatorInstruction}, - value::Value, + value::{Value, ValueId}, }, ssa_gen::Ssa, }; +use fxhash::FxHashMap as HashMap; + impl Ssa { /// Simplify each function's control flow graph by: /// 1. Removing blocks with no predecessors @@ -49,6 +51,7 @@ impl Function { /// be inlined into their predecessor. pub(crate) fn simplify_function(&mut self) { let mut cfg = ControlFlowGraph::with_function(self); + let mut values_to_replace = HashMap::default(); let mut stack = vec![self.entry_block()]; let mut visited = HashSet::new(); @@ -57,6 +60,10 @@ impl Function { stack.extend(self.dfg[block].successors().filter(|block| !visited.contains(block))); } + if !values_to_replace.is_empty() { + self.dfg.replace_values_in_block_instructions(block, &values_to_replace); + } + check_for_negated_jmpif_condition(self, block, &mut cfg); // This call is before try_inline_into_predecessor so that if it succeeds in changing a @@ -70,7 +77,7 @@ impl Function { drop(predecessors); // If the block has only 1 predecessor, we can safely remove its block parameters - remove_block_parameters(self, block, predecessor); + remove_block_parameters(self, block, predecessor, &mut values_to_replace); // Note: this function relies on `remove_block_parameters` being called first. // Otherwise the inlined block will refer to parameters that no longer exist. @@ -84,6 +91,15 @@ impl Function { check_for_double_jmp(self, block, &mut cfg); } + + if !values_to_replace.is_empty() { + self.dfg.replace_values_in_block_terminator(block, &values_to_replace); + } + } + + // Values from previous blocks might need to be replaced + for block in self.reachable_blocks() { + self.dfg.replace_values_in_block(block, &values_to_replace); } } } @@ -246,6 +262,7 @@ fn remove_block_parameters( function: &mut Function, block: BasicBlockId, predecessor: BasicBlockId, + values_to_replace: &mut HashMap, ) { let block = &mut function.dfg[block]; @@ -264,7 +281,7 @@ fn remove_block_parameters( assert_eq!(block_params.len(), jump_args.len()); for (param, arg) in block_params.iter().zip(jump_args) { - function.dfg.set_value_from_id(*param, arg); + values_to_replace.insert(*param, arg); } } } @@ -296,128 +313,53 @@ fn try_inline_into_predecessor( mod test { use crate::{ assert_ssa_snapshot, - ssa::{ - Ssa, - function_builder::FunctionBuilder, - ir::{ - instruction::{BinaryOp, TerminatorInstruction}, - map::Id, - types::Type, - }, - opt::assert_normalized_ssa_equals, - }, + ssa::{Ssa, opt::assert_normalized_ssa_equals}, }; - use acvm::acir::AcirField; #[test] fn inline_blocks() { - // fn main { - // b0(): - // jmp b1(Field 7) - // b1(v0: Field): - // jmp b2(v0) - // b2(v1: Field): - // return v1 - // } - let main_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("main".into(), main_id); - - let b1 = builder.insert_block(); - let b2 = builder.insert_block(); - - let v0 = builder.add_block_parameter(b1, Type::field()); - let v1 = builder.add_block_parameter(b2, Type::field()); - - let expected_return = 7u128; - let seven = builder.field_constant(expected_return); - builder.terminate_with_jmp(b1, vec![seven]); - - builder.switch_to_block(b1); - builder.terminate_with_jmp(b2, vec![v0]); - - builder.switch_to_block(b2); - builder.terminate_with_return(vec![v1]); - - let ssa = builder.finish(); - assert_eq!(ssa.main().reachable_blocks().len(), 3); + let src = " + acir(inline) fn main f0 { + b0(): + jmp b1(Field 7) + b1(v0: Field): + jmp b2(v0) + b2(v1: Field): + return v1 + } + "; + let ssa = Ssa::from_str(src).unwrap(); - // Expected output: - // fn main { - // b0(): - // return Field 7 - // } let ssa = ssa.simplify_cfg(); - let main = ssa.main(); - assert_eq!(main.reachable_blocks().len(), 1); - - match main.dfg[main.entry_block()].terminator() { - Some(TerminatorInstruction::Return { return_values, .. }) => { - assert_eq!(return_values.len(), 1); - let return_value = main - .dfg - .get_numeric_constant(return_values[0]) - .expect("Expected return value to be constant") - .to_u128(); - assert_eq!(return_value, expected_return); - } - other => panic!("Unexpected terminator {other:?}"), + assert_ssa_snapshot!(ssa, @r" + acir(inline) fn main f0 { + b0(): + return Field 7 } + "); } #[test] fn remove_known_jmpif() { - // fn main { - // b0(v0: u1): - // v1 = eq v0, v0 - // jmpif v1, then: b1, else: b2 - // b1(): - // return Field 1 - // b2(): - // return Field 2 - // } - let main_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("main".into(), main_id); - let v0 = builder.add_parameter(Type::bool()); - - let b1 = builder.insert_block(); - let b2 = builder.insert_block(); - - let one = builder.field_constant(1u128); - let two = builder.field_constant(2u128); - - let v1 = builder.insert_binary(v0, BinaryOp::Eq, v0); - builder.terminate_with_jmpif(v1, b1, b2); - - builder.switch_to_block(b1); - builder.terminate_with_return(vec![one]); - - builder.switch_to_block(b2); - builder.terminate_with_return(vec![two]); - - let ssa = builder.finish(); - assert_eq!(ssa.main().reachable_blocks().len(), 3); + let src = " + acir(inline) fn main f0 { + b0(v0: u1): + jmpif u1 1 then: b1, else: b2 + b1(): + return Field 1 + b2(): + return Field 2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); - // Expected output: - // fn main { - // b0(): - // return Field 1 - // } let ssa = ssa.simplify_cfg(); - let main = ssa.main(); - assert_eq!(main.reachable_blocks().len(), 1); - - match main.dfg[main.entry_block()].terminator() { - Some(TerminatorInstruction::Return { return_values, .. }) => { - assert_eq!(return_values.len(), 1); - let return_value = main - .dfg - .get_numeric_constant(return_values[0]) - .expect("Expected return value to be constant") - .to_u128(); - assert_eq!(return_value, 1u128); - } - other => panic!("Unexpected terminator {other:?}"), + assert_ssa_snapshot!(ssa, @r" + acir(inline) fn main f0 { + b0(v0: u1): + return Field 1 } + "); } #[test]