From 6ad5af8c0d76b8340dca33b7f1adeb40f69c8aa8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 13:55:10 -0300 Subject: [PATCH 01/18] chore: don't use `set_value_from_id` in `remove_truncate_after_range_checks` --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 42 +++++++++++++++++++ .../opt/remove_truncate_after_range_check.rs | 6 +-- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 690bb121634..9b5a9f37b53 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -383,6 +383,48 @@ impl DataFlowGraph { self.instructions[id] = instruction; } + /// Replaces all values in the given block 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_block( + &mut self, + block: BasicBlockId, + values_to_replace: &std::collections::HashMap, + ) { + if values_to_replace.is_empty() { + return; + } + + let replacement_fn = |value_id| { + if let Some(replacement_id) = values_to_replace.get(&value_id) { + *replacement_id + } else { + value_id + } + }; + + // 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; + } + } + } + } + + // 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); + } + } + /// Set the value of value_to_replace to refer to the value referred to by new_value. /// /// This is the preferred method to call for optimizations simplifying 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 8b8d6a05dc8..241fc9646ed 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 @@ -24,7 +24,7 @@ impl Function { // Keeps the minimum bit size a value was range-checked against let mut range_checks: HashMap = HashMap::new(); let mut instructions_to_remove = HashSet::new(); - let mut values_to_replace = HashMap::::new(); + let mut values_to_replace = std::collections::HashMap::::new(); for instruction_id in self.dfg[block].instructions() { let instruction = &self.dfg[*instruction_id]; @@ -70,9 +70,7 @@ impl Function { .instructions_mut() .retain(|instruction| !instructions_to_remove.contains(instruction)); - for (old_value, new_value) in values_to_replace { - self.dfg.set_value_from_id(old_value, new_value); - } + self.dfg.replace_values_in_block(block, &values_to_replace); } } } From 3a136fd7d234e870f0b5f8583b51021a60b14fc2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 14:10:32 -0300 Subject: [PATCH 02/18] Use FxHashMap --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- .../src/ssa/opt/remove_truncate_after_range_check.rs | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 9b5a9f37b53..180ab8c525d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -389,7 +389,7 @@ impl DataFlowGraph { pub(crate) fn replace_values_in_block( &mut self, block: BasicBlockId, - values_to_replace: &std::collections::HashMap, + values_to_replace: &HashMap, ) { if values_to_replace.is_empty() { return; 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 241fc9646ed..6e70e44a4a5 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 @@ -1,4 +1,5 @@ -use im::{HashMap, HashSet}; +use fxhash::FxHashMap as HashMap; +use fxhash::FxHashSet as HashSet; use crate::ssa::{ ir::{function::Function, instruction::Instruction, value::ValueId}, @@ -22,9 +23,9 @@ impl Function { pub(crate) fn remove_truncate_after_range_check(&mut self) { for block in self.reachable_blocks() { // Keeps the minimum bit size a value was range-checked against - let mut range_checks: HashMap = HashMap::new(); - let mut instructions_to_remove = HashSet::new(); - let mut values_to_replace = std::collections::HashMap::::new(); + let mut range_checks: HashMap = HashMap::default(); + let mut instructions_to_remove = HashSet::default(); + let mut values_to_replace = HashMap::::default(); for instruction_id in self.dfg[block].instructions() { let instruction = &self.dfg[*instruction_id]; From 0e4b91207df734cbc850e0c0dd023a9ca6f3ae2a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 14:41:11 -0300 Subject: [PATCH 03/18] Replace in all blocks --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 38 ++++++++++--------- .../opt/remove_truncate_after_range_check.rs | 12 ++++-- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 180ab8c525d..0c7427dc228 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -383,12 +383,12 @@ impl DataFlowGraph { self.instructions[id] = instruction; } - /// Replaces all values in the given block with the values in the given map. + /// 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_block( + pub(crate) fn replace_values_in_blocks( &mut self, - block: BasicBlockId, + blocks: impl Iterator, values_to_replace: &HashMap, ) { if values_to_replace.is_empty() { @@ -403,25 +403,27 @@ impl DataFlowGraph { } }; - // 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; + 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; + } } } } - } - // 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); + // 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); + } } } 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 6e70e44a4a5..9074585a9d8 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 @@ -21,11 +21,15 @@ impl Ssa { impl Function { pub(crate) fn remove_truncate_after_range_check(&mut self) { - for block in self.reachable_blocks() { + let mut values_to_replace = HashMap::::default(); + + let blocks = self.reachable_blocks(); + for block in &blocks { + let block = *block; + // Keeps the minimum bit size a value was range-checked against let mut range_checks: HashMap = HashMap::default(); let mut instructions_to_remove = HashSet::default(); - let mut values_to_replace = HashMap::::default(); for instruction_id in self.dfg[block].instructions() { let instruction = &self.dfg[*instruction_id]; @@ -70,9 +74,9 @@ impl Function { self.dfg[block] .instructions_mut() .retain(|instruction| !instructions_to_remove.contains(instruction)); - - self.dfg.replace_values_in_block(block, &values_to_replace); } + + self.dfg.replace_values_in_blocks(blocks.into_iter(), &values_to_replace); } } From ad73aa4856e3ca63620bb6b0db14422dda169fd4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 14:51:11 -0300 Subject: [PATCH 04/18] Make sure values are replaced across blocks, and fix optimization --- .../src/ssa/opt/remove_truncate_after_range_check.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 9074585a9d8..aeee55525f2 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 @@ -22,13 +22,12 @@ impl Ssa { impl Function { pub(crate) fn remove_truncate_after_range_check(&mut self) { let mut values_to_replace = HashMap::::default(); + // Keeps the minimum bit size a value was range-checked against + let mut range_checks: HashMap = HashMap::default(); let blocks = self.reachable_blocks(); for block in &blocks { let block = *block; - - // Keeps the minimum bit size a value was range-checked against - let mut range_checks: HashMap = HashMap::default(); let mut instructions_to_remove = HashSet::default(); for instruction_id in self.dfg[block].instructions() { @@ -94,6 +93,8 @@ mod tests { b0(v0: Field): range_check v0 to 64 bits // This is to make sure we keep the smallest one range_check v0 to 32 bits + jmp b1() + b1(): v1 = truncate v0 to 32 bits, max_bit_size: 254 return v1 } @@ -106,6 +107,8 @@ mod tests { b0(v0: Field): range_check v0 to 64 bits range_check v0 to 32 bits + jmp b1() + b1(): return v0 } "); From b49a9bbdc339c76d16204094aace2102c68a33cc Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 14:54:01 -0300 Subject: [PATCH 05/18] Add a comment --- .../src/ssa/opt/remove_truncate_after_range_check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 aeee55525f2..9273453d6cb 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 @@ -93,7 +93,7 @@ mod tests { b0(v0: Field): range_check v0 to 64 bits // This is to make sure we keep the smallest one range_check v0 to 32 bits - jmp b1() + jmp b1() // Make sure the optimization is applied across blocks b1(): v1 = truncate v0 to 32 bits, max_bit_size: 254 return v1 From 1eea96082ac59daacd11f6b90779e926c38af027 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 15:23:04 -0300 Subject: [PATCH 06/18] chore: don't use `set_value_from_id` in `as_slice_length` --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 26 +---------- .../src/ssa/opt/as_slice_length.rs | 44 ++++++++++++------- 2 files changed, 28 insertions(+), 42 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 0c7427dc228..1534e8a1b5a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -82,7 +82,7 @@ pub(crate) struct DataFlowGraph { foreign_functions: HashMap, /// All blocks in a function - blocks: DenseMap, + pub(crate) blocks: DenseMap, /// Debugging information about which `ValueId`s have had their underlying `Value` substituted /// for that of another. In theory this information is purely used for printing the SSA, @@ -603,30 +603,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/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 837a35126ac..19c031d02ac 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -1,5 +1,8 @@ +use std::collections::BTreeSet; + use crate::ssa::{ ir::{ + basic_block::BasicBlockId, function::Function, instruction::{Instruction, InstructionId, Intrinsic}, types::{NumericType, Type}, @@ -28,17 +31,23 @@ 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 reachable_blocks = self.reachable_blocks(); + let known_slice_lengths = known_slice_lengths(self, &reachable_blocks); + replace_known_slice_lengths(self, known_slice_lengths, reachable_blocks); } } -fn known_slice_lengths(func: &Function) -> HashMap { +fn known_slice_lengths( + func: &Function, + reachable_blocks: &BTreeSet, +) -> HashMap { let mut known_slice_lengths = HashMap::default(); - for block_id in func.reachable_blocks() { - let block = &func.dfg[block_id]; + for block_id in reachable_blocks { + let block = &func.dfg[*block_id]; for instruction_id in block.instructions() { - let (target_func, arguments) = match &func.dfg[*instruction_id] { + let instruction = &func.dfg[*instruction_id]; + + let (target_func, arguments) = match &instruction { Instruction::Call { func, arguments } => (func, arguments), _ => continue, }; @@ -62,18 +71,19 @@ fn known_slice_lengths(func: &Function) -> HashMap { fn replace_known_slice_lengths( func: &mut Function, known_slice_lengths: HashMap, + reachable_blocks: BTreeSet, ) { - 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); - }); + let values_to_replace = known_slice_lengths + .into_iter() + .map(|(instruction_id, known_length)| { + let call_returns = func.dfg.instruction_results(instruction_id); + let original_slice_length = call_returns[0]; + let known_length = + func.dfg.make_constant(known_length.into(), NumericType::length_type()); + (original_slice_length, known_length) + }) + .collect::>(); + func.dfg.replace_values_in_blocks(reachable_blocks.into_iter(), &values_to_replace); } #[cfg(test)] From b990e295431550db850f27ef4fd3be23da99d4fb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 15:43:25 -0300 Subject: [PATCH 07/18] Undo visibility change --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 1534e8a1b5a..d23c1b44d68 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -82,7 +82,7 @@ pub(crate) struct DataFlowGraph { foreign_functions: HashMap, /// All blocks in a function - pub(crate) blocks: DenseMap, + blocks: DenseMap, /// Debugging information about which `ValueId`s have had their underlying `Value` substituted /// for that of another. In theory this information is purely used for printing the SSA, From 3f81295c112d21e29ca7f1590ad5a82c634f6a17 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 16:20:32 -0300 Subject: [PATCH 08/18] Don't set constants in result positions --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index d23c1b44d68..07069ec2e16 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -19,6 +19,7 @@ use super::{ use acvm::{FieldElement, acir::AcirField}; use fxhash::FxHashMap as HashMap; +use fxhash::FxHashSet as HashSet; use iter_extended::vecmap; use serde::{Deserialize, Serialize}; use serde_with::DisplayFromStr; @@ -395,6 +396,11 @@ impl DataFlowGraph { return; } + let constants = values_to_replace + .values() + .filter(|value_id| self.is_constant(**value_id)) + .collect::>(); + let replacement_fn = |value_id| { if let Some(replacement_id) = values_to_replace.get(&value_id) { *replacement_id @@ -414,7 +420,17 @@ impl DataFlowGraph { if let Some(results) = results { for result in results { if let Some(replacement_id) = values_to_replace.get(result) { - *result = *replacement_id; + // Don't replace results with constants as this leads to errors later on. + // For example, this: + // + // v1, v2 = call as_slice(v2) -> (u32, [u8; 3]) + // + // should not be replaced like this: + // + // u32 3, v2 = call as_slice(v2) -> (u32, [u8; 3]) + if !constants.contains(replacement_id) { + *result = *replacement_id; + } } } } From a31a4184b0b4916479c98dc66067a8fbbb590b34 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 16:34:08 -0300 Subject: [PATCH 09/18] Bring back `replace_result` --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 24 +++++++++++++++++++ .../src/ssa/opt/as_slice_length.rs | 9 +++++++ 2 files changed, 33 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 07069ec2e16..2e70a59134f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -619,6 +619,30 @@ 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/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 19c031d02ac..d1d299565ac 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -73,6 +73,15 @@ fn replace_known_slice_lengths( known_slice_lengths: HashMap, reachable_blocks: BTreeSet, ) { + // 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. + for instruction_id in known_slice_lengths.keys() { + let call_returns = func.dfg.instruction_results(*instruction_id); + let original_slice_length = call_returns[0]; + func.dfg.replace_result(*instruction_id, original_slice_length); + } + let values_to_replace = known_slice_lengths .into_iter() .map(|(instruction_id, known_length)| { From 30677258b3ed81ec218155e2daa71a41a921be93 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 16:40:21 -0300 Subject: [PATCH 10/18] Revert "Bring back `replace_result`" This reverts commit a31a4184b0b4916479c98dc66067a8fbbb590b34. --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 24 ------------------- .../src/ssa/opt/as_slice_length.rs | 9 ------- 2 files changed, 33 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 2e70a59134f..07069ec2e16 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -619,30 +619,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/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index d1d299565ac..19c031d02ac 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs @@ -73,15 +73,6 @@ fn replace_known_slice_lengths( known_slice_lengths: HashMap, reachable_blocks: BTreeSet, ) { - // 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. - for instruction_id in known_slice_lengths.keys() { - let call_returns = func.dfg.instruction_results(*instruction_id); - let original_slice_length = call_returns[0]; - func.dfg.replace_result(*instruction_id, original_slice_length); - } - let values_to_replace = known_slice_lengths .into_iter() .map(|(instruction_id, known_length)| { From ebb6e7aa5568f55ee3d3607c85f6736c09ec979e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Apr 2025 20:31:51 -0300 Subject: [PATCH 11/18] No need to replace instruction results --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 26 ---------------------- 1 file changed, 26 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 07069ec2e16..6839defd978 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -19,7 +19,6 @@ use super::{ use acvm::{FieldElement, acir::AcirField}; use fxhash::FxHashMap as HashMap; -use fxhash::FxHashSet as HashSet; use iter_extended::vecmap; use serde::{Deserialize, Serialize}; use serde_with::DisplayFromStr; @@ -396,11 +395,6 @@ impl DataFlowGraph { return; } - let constants = values_to_replace - .values() - .filter(|value_id| self.is_constant(**value_id)) - .collect::>(); - let replacement_fn = |value_id| { if let Some(replacement_id) = values_to_replace.get(&value_id) { *replacement_id @@ -414,26 +408,6 @@ impl DataFlowGraph { 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) { - // Don't replace results with constants as this leads to errors later on. - // For example, this: - // - // v1, v2 = call as_slice(v2) -> (u32, [u8; 3]) - // - // should not be replaced like this: - // - // u32 3, v2 = call as_slice(v2) -> (u32, [u8; 3]) - if !constants.contains(replacement_id) { - *result = *replacement_id; - } - } - } - } } // Finally, the value might show up in a terminator From 7a8697372284a6115a0c17a55efd357a9aa642c9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 10:02:07 -0300 Subject: [PATCH 12/18] Do `as_slice_length` optimization in one pass --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 31 ++++--- .../noirc_evaluator/src/ssa/ir/instruction.rs | 16 ++++ .../src/ssa/opt/as_slice_length.rs | 85 ++++++++----------- 3 files changed, 69 insertions(+), 63 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 6839defd978..d6c3d4dc9c0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -395,25 +395,32 @@ impl DataFlowGraph { return; } - 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); + instruction.replace_values(values_to_replace); } // 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); - } + self.replace_values_in_block_terminator(block, values_to_replace); + } + } + + /// 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| { + if let Some(replacement_id) = values_to_replace.get(&value_id) { + *replacement_id + } else { + value_id + } + }); } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index ee4adfe89e2..bafff2a785b 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}; @@ -422,6 +423,21 @@ 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() { + return; + } + + self.map_values_mut(|value_id| { + if let Some(replacement_id) = values_to_replace.get(&value_id) { + *replacement_id + } else { + 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/opt/as_slice_length.rs b/compiler/noirc_evaluator/src/ssa/opt/as_slice_length.rs index 19c031d02ac..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,10 +1,7 @@ -use std::collections::BTreeSet; - use crate::ssa::{ ir::{ - basic_block::BasicBlockId, function::Function, - instruction::{Instruction, InstructionId, Intrinsic}, + instruction::{Instruction, Intrinsic}, types::{NumericType, Type}, value::Value, }, @@ -31,59 +28,45 @@ impl Ssa { impl Function { pub(crate) fn as_slice_optimization(&mut self) { - let reachable_blocks = self.reachable_blocks(); - let known_slice_lengths = known_slice_lengths(self, &reachable_blocks); - replace_known_slice_lengths(self, known_slice_lengths, reachable_blocks); - } -} + 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, - reachable_blocks: &BTreeSet, -) -> HashMap { - let mut known_slice_lengths = HashMap::default(); - for block_id in reachable_blocks { - let block = &func.dfg[*block_id]; - for instruction_id in block.instructions() { - let instruction = &func.dfg[*instruction_id]; + let (target_func, arguments) = match &instruction { + Instruction::Call { func, arguments } => (func, arguments), + _ => continue, + }; - let (target_func, arguments) = match &instruction { - Instruction::Call { func, arguments } => (func, arguments), - _ => continue, - }; + (*target_func, arguments.first().copied()) + }; - 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); + 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, - reachable_blocks: BTreeSet, -) { - let values_to_replace = known_slice_lengths - .into_iter() - .map(|(instruction_id, known_length)| { - let call_returns = func.dfg.instruction_results(instruction_id); - let original_slice_length = call_returns[0]; - let known_length = - func.dfg.make_constant(known_length.into(), NumericType::length_type()); - (original_slice_length, known_length) - }) - .collect::>(); - func.dfg.replace_values_in_blocks(reachable_blocks.into_iter(), &values_to_replace); } #[cfg(test)] From cd294fd0223c3dfe531e2fd44c992aedceed333f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 10:09:11 -0300 Subject: [PATCH 13/18] Implement `remove_truncate_after_range_check` in one go --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 24 ------------------- .../opt/remove_truncate_after_range_check.rs | 21 ++++++++-------- 2 files changed, 11 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index d6c3d4dc9c0..e5f39773392 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -383,30 +383,6 @@ 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( - &mut self, - blocks: impl Iterator, - values_to_replace: &HashMap, - ) { - if values_to_replace.is_empty() { - return; - } - - 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.replace_values(values_to_replace); - } - - // Finally, the value might show up in a terminator - self.replace_values_in_block_terminator(block, values_to_replace); - } - } - /// 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, 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); } } From 9370ed1e42bc902d41ffe9a4bd59eadb88e1c57b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 14:07:40 -0300 Subject: [PATCH 14/18] Use SSA parser tests in simplify_cfg --- .../src/ssa/opt/simplify_cfg.rs | 141 ++++-------------- 1 file changed, 33 insertions(+), 108 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index b042cf9a41e..990c15d9e27 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -296,128 +296,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] From 294dd1836ab3eda686fc68978baaf9674eefe1e8 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 14:08:21 -0300 Subject: [PATCH 15/18] Recursively solve values --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 10 +++------- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 14 +++----------- compiler/noirc_evaluator/src/ssa/ir/value.rs | 12 ++++++++++++ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index e5f39773392..c2f7ac83015 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -390,13 +390,9 @@ impl DataFlowGraph { values_to_replace: &HashMap, ) { if self[block].terminator().is_some() { - self[block].unwrap_terminator_mut().map_values_mut(|value_id| { - if let Some(replacement_id) = values_to_replace.get(&value_id) { - *replacement_id - } else { - value_id - } - }); + self[block] + .unwrap_terminator_mut() + .map_values_mut(|value_id| resolve_value(values_to_replace, value_id)); } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index bafff2a785b..ce6eb6457fe 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -15,7 +15,7 @@ use super::{ dfg::DataFlowGraph, map::Id, types::{NumericType, Type}, - value::{Value, ValueId}, + value::{Value, ValueId, resolve_value}, }; pub(crate) mod binary; @@ -425,17 +425,9 @@ 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() { - return; + if !values_to_replace.is_empty() { + self.map_values_mut(|value_id| resolve_value(values_to_replace, value_id)); } - - self.map_values_mut(|value_id| { - if let Some(replacement_id) = values_to_replace.get(&value_id) { - *replacement_id - } else { - value_id - } - }); } /// Maps each ValueId inside this instruction to a new ValueId, returning the new instruction. 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 + } +} From 75865fbe4aefee26810362e464d44396fe361db9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 14:08:37 -0300 Subject: [PATCH 16/18] Don't use `set_value_from_id` in `simplify_cfg` --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 26 ++++++++++++++++++- .../src/ssa/opt/simplify_cfg.rs | 23 +++++++++++++--- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index c2f7ac83015..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,6 +383,30 @@ impl DataFlowGraph { self.instructions[id] = instruction; } + /// 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, + block: BasicBlockId, + values_to_replace: &HashMap, + ) { + self.replace_values_in_block_instructions(block, values_to_replace); + self.replace_values_in_block_terminator(block, values_to_replace); + } + + /// 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; + } + /// 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, diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 990c15d9e27..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); } } } From a33eaa8b693b3d786a35bfbaaf9cb78f6d3c197b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 14:08:21 -0300 Subject: [PATCH 17/18] Recursively solve values --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 10 +++------- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 14 +++----------- compiler/noirc_evaluator/src/ssa/ir/value.rs | 12 ++++++++++++ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index e5f39773392..c2f7ac83015 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -390,13 +390,9 @@ impl DataFlowGraph { values_to_replace: &HashMap, ) { if self[block].terminator().is_some() { - self[block].unwrap_terminator_mut().map_values_mut(|value_id| { - if let Some(replacement_id) = values_to_replace.get(&value_id) { - *replacement_id - } else { - value_id - } - }); + self[block] + .unwrap_terminator_mut() + .map_values_mut(|value_id| resolve_value(values_to_replace, value_id)); } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index bafff2a785b..ce6eb6457fe 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -15,7 +15,7 @@ use super::{ dfg::DataFlowGraph, map::Id, types::{NumericType, Type}, - value::{Value, ValueId}, + value::{Value, ValueId, resolve_value}, }; pub(crate) mod binary; @@ -425,17 +425,9 @@ 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() { - return; + if !values_to_replace.is_empty() { + self.map_values_mut(|value_id| resolve_value(values_to_replace, value_id)); } - - self.map_values_mut(|value_id| { - if let Some(replacement_id) = values_to_replace.get(&value_id) { - *replacement_id - } else { - value_id - } - }); } /// Maps each ValueId inside this instruction to a new ValueId, returning the new instruction. 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 + } +} From 34f8e08504b1c3c38b610a9333fea46e9d0b79cb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 14 Apr 2025 14:12:42 -0300 Subject: [PATCH 18/18] clippy --- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index c2f7ac83015..ad386b6e6df 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};