fix(ssa): Remove the array cache from the function inserter

compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs

@@ -1,8 +1,6 @@
 use iter_extended::vecmap;
 use noirc_errors::call_stack::CallStackId;
 
-use crate::ssa::ir::types::Type;
-
 use super::{
     basic_block::BasicBlockId,
     dfg::InsertInstructionResult,
@@ -19,26 +17,11 @@ pub(crate) struct FunctionInserter<'f> {
     pub(crate) function: &'f mut Function,
 
     values: HashMap<ValueId, ValueId>,
-
-    /// Map containing repeat array constants so that we do not initialize a new
-    /// array unnecessarily. An extra tuple field is included as part of the key to
-    /// distinguish between array/slice types.
-    ///
-    /// This is optional since caching arrays relies on the inserter inserting strictly
-    /// in control-flow order. Otherwise, if arrays later in the program are cached first,
-    /// they may be referred to by instructions earlier in the program.
-    array_cache: Option<ArrayCache>,
-
-    /// If this pass is loop unrolling, store the block before the loop to optionally
-    /// hoist any make_array instructions up to after they are retrieved from the `array_cache`.
-    pre_loop: Option<BasicBlockId>,
 }
 
-pub(crate) type ArrayCache = HashMap<im::Vector<ValueId>, HashMap<Type, ValueId>>;
-
 impl<'f> FunctionInserter<'f> {
     pub(crate) fn new(function: &'f mut Function) -> FunctionInserter<'f> {
-        Self { function, values: HashMap::default(), array_cache: None, pre_loop: None }
+        Self { function, values: HashMap::default() }
     }
 
     /// Resolves a ValueId to its new, updated value.
@@ -120,7 +103,7 @@ impl<'f> FunctionInserter<'f> {
         &mut self,
         instruction: Instruction,
         id: InstructionId,
-        mut block: BasicBlockId,
+        block: BasicBlockId,
         call_stack: CallStackId,
     ) -> InsertInstructionResult {
         let results = self.function.dfg.instruction_results(id).to_vec();
@@ -129,85 +112,17 @@ impl<'f> FunctionInserter<'f> {
             .requires_ctrl_typevars()
             .then(|| vecmap(&results, |result| self.function.dfg.type_of_value(*result)));
 
-        // Large arrays can lead to OOM panics if duplicated from being unrolled in loops.
-        // To prevent this, try to reuse the same ID for identical arrays instead of inserting
-        // another MakeArray instruction. Note that this assumes the function inserter is inserting
-        // in control-flow order. Otherwise we could refer to ValueIds defined later in the program.
-        let make_array = if let Instruction::MakeArray { elements, typ } = &instruction {
-            if self.array_is_constant(elements) && self.function.runtime().is_acir() {
-                if let Some(fetched_value) = self.get_cached_array(elements, typ) {
-                    assert_eq!(results.len(), 1);
-                    self.values.insert(results[0], fetched_value);
-                    return InsertInstructionResult::SimplifiedTo(fetched_value);
-                }
-
-                // Hoist constant arrays out of the loop and cache their value
-                if let Some(pre_loop) = self.pre_loop {
-                    block = pre_loop;
-                }
-                Some((elements.clone(), typ.clone()))
-            } else {
-                None
-            }
-        } else {
-            None
-        };
-
         let new_results = self.function.dfg.insert_instruction_and_results(
             instruction,
             block,
             ctrl_typevars,
             call_stack,
         );
 
-        // Cache an array in the fresh_array_cache if array caching is enabled.
-        // The fresh cache isn't used for deduplication until an external pass confirms we
-        // pass a sequence point and all blocks that may be before the current insertion point
-        // are finished.
-        if let Some((elements, typ)) = make_array {
-            Self::cache_array(&mut self.array_cache, elements, typ, new_results.first());
-        }
-
         Self::insert_new_instruction_results(&mut self.values, &results, &new_results);
         new_results
     }
 
-    fn get_cached_array(&self, elements: &im::Vector<ValueId>, typ: &Type) -> Option<ValueId> {
-        self.array_cache.as_ref()?.get(elements)?.get(typ).copied()
-    }
-
-    fn cache_array(
-        arrays: &mut Option<ArrayCache>,
-        elements: im::Vector<ValueId>,
-        typ: Type,
-        result_id: ValueId,
-    ) {
-        if let Some(arrays) = arrays {
-            arrays.entry(elements).or_default().insert(typ, result_id);
-        }
-    }
-
-    fn array_is_constant(&self, elements: &im::Vector<ValueId>) -> bool {
-        elements.iter().all(|element| self.function.dfg.is_constant(*element))
-    }
-
-    pub(crate) fn set_array_cache(
-        &mut self,
-        new_cache: Option<ArrayCache>,
-        pre_loop: BasicBlockId,
-    ) {
-        self.array_cache = new_cache;
-        self.pre_loop = Some(pre_loop);
-    }
-
-    /// Finish this inserter, returning its array cache merged with the fresh array cache.
-    /// Since this consumes the inserter this assumes we're at a sequence point where all
-    /// predecessor blocks to the current block are finished. Since this is true, the fresh
-    /// array cache can be merged with the existing array cache.
-    pub(crate) fn into_array_cache(self) -> Option<ArrayCache> {
-        self.array_cache
-    }
-
     /// Modify the values HashMap to remember the mapping between an instruction result's previous
     /// ValueId (from the source_function) and its new ValueId in the destination function.
     pub(crate) fn insert_new_instruction_results(

compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs

@@ -178,7 +178,7 @@ impl IdMaps {
             value @ Value::Instruction { instruction, .. } => {
                 *self.values.get(&old_value).unwrap_or_else(|| {
                     let instruction = &old_function.dfg[*instruction];
-                    unreachable!("Unmapped value with id {old_value}: {value:?}\n  from instruction: {instruction:?}, SSA: {old_function}")
+                    unreachable!("Unmapped value with id {old_value}: {value:?}\n  from instruction: {instruction:?}, from function: {}", old_function.id())
                 })
             }
 

compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

@@ -33,7 +33,7 @@ use crate::{
             dfg::DataFlowGraph,
             dom::DominatorTree,
             function::Function,
-            function_inserter::{ArrayCache, FunctionInserter},
+            function_inserter::FunctionInserter,
             instruction::{Binary, BinaryOp, Instruction, InstructionId, TerminatorInstruction},
             integer::IntegerConstant,
             post_order::PostOrder,
@@ -439,21 +439,9 @@ impl Loop {
     fn unroll(&self, function: &mut Function, cfg: &ControlFlowGraph) -> Result<(), CallStack> {
         let mut unroll_into = self.get_pre_header(function, cfg)?;
         let mut jump_value = get_induction_variable(function, unroll_into)?;
-        let mut array_cache = Some(ArrayCache::default());
-
-        while let Some(mut context) = self.unroll_header(function, unroll_into, jump_value)? {
-            // The inserter's array cache must be explicitly enabled. This is to
-            // confirm that we're inserting in insertion order. This is true here since:
-            // 1. We have a fresh inserter for each loop
-            // 2. Each loop is unrolled in iteration order
-            //
-            // Within a loop we do not insert in insertion order. This is fine however since the
-            // array cache is buffered with a separate fresh_array_cache which collects arrays
-            // but does not deduplicate. When we later call `into_array_cache`, that will merge
-            // the fresh cache in with the old one so that each iteration of the loop can cache
-            // from previous iterations but not the current iteration.
-            context.inserter.set_array_cache(array_cache, unroll_into);
-            (unroll_into, jump_value, array_cache) = context.unroll_loop_iteration();
+
+        while let Some(context) = self.unroll_header(function, unroll_into, jump_value)? {
+            (unroll_into, jump_value) = context.unroll_loop_iteration();
         }
 
         Ok(())
@@ -872,7 +860,7 @@ impl<'f> LoopIteration<'f> {
     /// It is expected the terminator instructions are set up to branch into an empty block
     /// for further unrolling. When the loop is finished this will need to be mutated to
     /// jump to the end of the loop instead.
-    fn unroll_loop_iteration(mut self) -> (BasicBlockId, ValueId, Option<ArrayCache>) {
+    fn unroll_loop_iteration(mut self) -> (BasicBlockId, ValueId) {
         let mut next_blocks = self.unroll_loop_block();
 
         while let Some(block) = next_blocks.pop() {
@@ -890,7 +878,7 @@ impl<'f> LoopIteration<'f> {
             .induction_value
             .expect("Expected to find the induction variable by end of loop iteration");
 
-        (end_block, induction_value, self.inserter.into_array_cache())
+        (end_block, induction_value)
     }
 
     /// Unroll a single block in the current iteration of the loop

test_programs/execution_success/unrolling_regression_8333/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "unrolling_regression_8333"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_success/unrolling_regression_8333/src/main.nr

@@ -0,0 +1,9 @@
+// Regression for issue #8333 (https://github.com/noir-lang/noir/issues/8333)
+fn main() -> pub [[u8; 32]; 2] {
+    let mut result = [[0; 32]; 2];
+    let actions_data: [Field; 30] = [0; 30];
+    for i in 0..1 {
+        result = [actions_data[i].to_be_bytes::<32>(), actions_data[i + 1].to_be_bytes::<32>()];
+    }
+    result
+}

test_programs/execution_success/unrolling_regression_8333/stdout.txt

@@ -0,0 +1 @@
+[unrolling_regression_8333] Circuit output: Vec([Vec([Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0)]), Vec([Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0), Field(0)])])