chore: don't use set_value_from_id in simplify_cfg

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<Item = BasicBlockId>,
+        block: BasicBlockId,
         values_to_replace: &HashMap<ValueId, ValueId>,
     ) {
-        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<ValueId, ValueId>,
+    ) {
+        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<ValueId, ValueId>,
+    ) {
+        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()

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<ValueId, ValueId>) {
+        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.

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<ValueId, ValueId>,
+    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
+    }
+}

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<InstructionId, u32> {
-    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<InstructionId, u32>,
-) {
-    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)]

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);
     }
 }