diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index ed3accae15a..865a1e31eb3 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -28,14 +28,21 @@ impl Ssa { impl Function { pub(crate) fn array_set_optimization(&mut self) { + if matches!(self.runtime(), RuntimeType::Brillig(_)) { + // Brillig is supposed to use refcounting to decide whether to mutate an array; + // array mutation was only meant for ACIR. We could use it with Brillig as well, + // but then some of the optimizations that we can do in ACIR around shared + // references have to be skipped, which makes it more cumbersome. + return; + } + let reachable_blocks = self.reachable_blocks(); if !self.runtime().is_entry_point() { assert_eq!(reachable_blocks.len(), 1, "Expected there to be 1 block remaining in Acir function for array_set optimization"); } - let mut context = - Context::new(&self.dfg, matches!(self.runtime(), RuntimeType::Brillig(_))); + let mut context = Context::new(&self.dfg); for block in reachable_blocks.iter() { context.analyze_last_uses(*block); @@ -50,20 +57,18 @@ impl Function { struct Context<'f> { dfg: &'f DataFlowGraph, - is_brillig_runtime: bool, array_to_last_use: HashMap, instructions_that_can_be_made_mutable: HashSet, // Mapping of an array that comes from a load and whether the address - // it was loaded from is a reference parameter. + // it was loaded from is a reference parameter passed to the block. arrays_from_load: HashMap, inner_nested_arrays: HashMap, } impl<'f> Context<'f> { - fn new(dfg: &'f DataFlowGraph, is_brillig_runtime: bool) -> Self { + fn new(dfg: &'f DataFlowGraph) -> Self { Context { dfg, - is_brillig_runtime, array_to_last_use: HashMap::default(), instructions_that_can_be_made_mutable: HashSet::default(), arrays_from_load: HashMap::default(), @@ -85,43 +90,41 @@ impl<'f> Context<'f> { self.instructions_that_can_be_made_mutable.remove(&existing); } } - Instruction::ArraySet { array, value, .. } => { + Instruction::ArraySet { array, .. } => { let array = self.dfg.resolve(*array); if let Some(existing) = self.array_to_last_use.insert(array, *instruction_id) { self.instructions_that_can_be_made_mutable.remove(&existing); } - if self.is_brillig_runtime { - let value = self.dfg.resolve(*value); - - if let Some(existing) = self.inner_nested_arrays.get(&value) { - self.instructions_that_can_be_made_mutable.remove(existing); - } - let result = self.dfg.instruction_results(*instruction_id)[0]; - self.inner_nested_arrays.insert(result, *instruction_id); - } // If the array we are setting does not come from a load we can safely mark it mutable. // If the array comes from a load we may potentially being mutating an array at a reference // that is loaded from by other values. let terminator = self.dfg[block_id].unwrap_terminator(); + // If we are in a return block we are not concerned about the array potentially being mutated again. let is_return_block = matches!(terminator, TerminatorInstruction::Return { .. }); + // We also want to check that the array is not part of the terminator arguments, as this means it is used again. - let mut array_in_terminator = false; + let mut is_array_in_terminator = false; terminator.for_each_value(|value| { - if value == array { - array_in_terminator = true; + // The terminator can contain original IDs, while the SSA has replaced the array value IDs; we need to resolve to compare. + if !is_array_in_terminator && self.dfg.resolve(value) == array { + is_array_in_terminator = true; } }); - if let Some(is_from_param) = self.arrays_from_load.get(&array) { + + let can_mutate = if let Some(is_from_param) = self.arrays_from_load.get(&array) + { // If the array was loaded from a reference parameter, we cannot // safely mark that array mutable as it may be shared by another value. - if !is_from_param && is_return_block { - self.instructions_that_can_be_made_mutable.insert(*instruction_id); - } - } else if !array_in_terminator { + !is_from_param && is_return_block + } else { + !is_array_in_terminator + }; + + if can_mutate { self.instructions_that_can_be_made_mutable.insert(*instruction_id); } }