diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 1fa4985295a..a7fb1418d60 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -466,6 +466,7 @@ impl<'block> BrilligBlock<'block> { destination_vector, source_size_register, None, + None, ); // Items @@ -813,13 +814,14 @@ impl<'block> BrilligBlock<'block> { // Initialize the variable match new_variable { BrilligVariable::BrilligArray(brillig_array) => { - self.brillig_context.codegen_initialize_array(brillig_array); + self.brillig_context.codegen_initialize_array(brillig_array, None); } BrilligVariable::BrilligVector(vector) => { let size = self .brillig_context .make_usize_constant_instruction(array.len().into()); - self.brillig_context.codegen_initialize_vector(vector, size, None); + self.brillig_context + .codegen_initialize_vector(vector, size, None, None); self.brillig_context.deallocate_single_addr(size); } _ => unreachable!( @@ -1814,7 +1816,7 @@ impl<'block> BrilligBlock<'block> { unreachable!("ICE: allocate_foreign_call_array() expects an array, got {typ:?}") }; - self.brillig_context.codegen_initialize_array(array); + self.brillig_context.codegen_initialize_array(array, None); let mut index = 0_usize; for _ in 0..*size { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs index ba89823ef13..20f87858f1b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs @@ -75,7 +75,7 @@ impl BrilligContext< assert!(source_field.bit_size == F::max_num_bits()); assert!(radix.bit_size == 32); - self.codegen_initialize_array(target_array); + self.codegen_initialize_array(target_array, None); let heap_array = self.codegen_brillig_array_to_heap_array(target_array); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs index a34034bb550..130f5906232 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs @@ -12,6 +12,8 @@ use super::{ BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; +const INITIAL_ARRAY_REF_COUNT: usize = 0; + impl BrilligContext { /// Allocates an array of size `size` and stores the pointer to the array /// in `pointer_register` @@ -419,12 +421,16 @@ impl BrilligContext< } /// Initializes an array, allocating memory to store its representation and initializing the reference counter. - pub(crate) fn codegen_initialize_array(&mut self, array: BrilligArray) { + pub(crate) fn codegen_initialize_array( + &mut self, + array: BrilligArray, + initial_rc: Option, + ) { self.codegen_allocate_immediate_mem(array.pointer, array.size + 1); self.indirect_const_instruction( array.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), + initial_rc.unwrap_or(INITIAL_ARRAY_REF_COUNT).into(), ); } @@ -433,12 +439,13 @@ impl BrilligContext< vector: BrilligVector, size: SingleAddrVariable, capacity: Option, + initial_rc: Option, ) { // Write RC self.indirect_const_instruction( vector.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), + initial_rc.unwrap_or(INITIAL_ARRAY_REF_COUNT).into(), ); // Write size @@ -459,6 +466,7 @@ impl BrilligContext< vector: BrilligVector, size: SingleAddrVariable, capacity: Option, // Defaults to size if None + initial_rc: Option, // Defaults to INITIAL_ARRAY_REF_COUNT if none ) { let allocation_size = self.allocate_register(); // Allocation size = capacity + 3 (rc, size, capacity) @@ -471,7 +479,7 @@ impl BrilligContext< self.codegen_allocate_mem(vector.pointer, allocation_size); self.deallocate_register(allocation_size); - self.codegen_initialize_vector_metadata(vector, size, capacity); + self.codegen_initialize_vector_metadata(vector, size, capacity, initial_rc); } /// We don't know the length of a vector returned externally before the call @@ -498,7 +506,7 @@ impl BrilligContext< self.indirect_const_instruction( vector.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), + INITIAL_ARRAY_REF_COUNT.into(), ); // Initialize size diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 2dbee48b277..cfeb787bc4c 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -199,7 +199,7 @@ impl BrilligContext { let deflattened_items_pointer = if is_vector { let vector = BrilligVector { pointer: deflattened_array_pointer }; - self.codegen_initialize_vector(vector, deflattened_size_variable, None); + self.codegen_initialize_vector(vector, deflattened_size_variable, None, Some(1)); self.codegen_make_vector_items_pointer(vector) } else { @@ -207,7 +207,7 @@ impl BrilligContext { pointer: deflattened_array_pointer, size: item_count * item_type.len(), }; - self.codegen_initialize_array(arr); + self.codegen_initialize_array(arr, Some(1)); self.codegen_make_array_items_pointer(arr) }; diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_push.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_push.rs index 798cf2385e5..0f2a679c2c1 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_push.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_push.rs @@ -185,6 +185,7 @@ pub(crate) fn reallocate_vector_for_insertion< target_vector, target_size, Some(source_capacity), + Some(1), ); } }); @@ -201,6 +202,7 @@ pub(crate) fn reallocate_vector_for_insertion< target_vector, target_size, Some(double_size), + Some(1), ); brillig_context.deallocate_single_addr(double_size); } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs index bfc9d512852..8475727c273 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs @@ -78,7 +78,7 @@ pub(super) fn compile_vector_pop_back_procedure( brillig_context.codegen_update_vector_length(target_vector, target_size); } else { // We need to clone the source vector - brillig_context.codegen_initialize_vector(target_vector, target_size, None); + brillig_context.codegen_initialize_vector(target_vector, target_size, None, Some(1)); let target_vector_items_pointer = brillig_context.codegen_make_vector_items_pointer(target_vector); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs index 49123ca2f50..39615e25ea5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs @@ -95,9 +95,10 @@ pub(super) fn compile_vector_pop_front_procedure( target_vector, target_size, Some(source_capacity), + Some(1), ); } else { - brillig_context.codegen_initialize_vector(target_vector, target_size, None); + brillig_context.codegen_initialize_vector(target_vector, target_size, None, Some(1)); let target_vector_items_pointer = brillig_context.codegen_make_vector_items_pointer(target_vector); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs index 7abc43286ee..9785c5cc4fe 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs @@ -81,7 +81,7 @@ pub(super) fn compile_vector_remove_procedure( brillig_context .codegen_vector_items_pointer(target_vector, target_vector_items_pointer); } else { - brillig_context.codegen_initialize_vector(target_vector, target_size, None); + brillig_context.codegen_initialize_vector(target_vector, target_size, None, Some(1)); // Copy the elements to the left of the index brillig_context diff --git a/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index a0c23ad70aa..74f5d2bf85a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -129,7 +129,8 @@ impl<'f> FunctionInserter<'f> { // 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) { + // Array caching is disabled for brillig functions since they may be mutated directly + 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); diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 0d7b9d10f4c..1f14faedd98 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -424,7 +424,7 @@ impl Instruction { /// conditional on whether the caller wants the predicate to be taken into account or not. pub(crate) fn can_be_deduplicated( &self, - dfg: &DataFlowGraph, + function: &Function, deduplicate_with_predicate: bool, ) -> bool { use Instruction::*; @@ -438,7 +438,7 @@ impl Instruction { | IncrementRc { .. } | DecrementRc { .. } => false, - Call { func, .. } => match dfg[*func] { + Call { func, .. } => match function.dfg[*func] { Value::Intrinsic(intrinsic) => { intrinsic.can_be_deduplicated(deduplicate_with_predicate) } @@ -448,8 +448,8 @@ impl Instruction { // We can deduplicate these instructions if we know the predicate is also the same. Constrain(..) | RangeCheck { .. } => deduplicate_with_predicate, - // This should never be side-effectful - MakeArray { .. } => true, + // Arrays can be mutated in unconstrained code so we can't deduplicate there + MakeArray { .. } => function.runtime().is_acir(), // These can have different behavior depending on the EnableSideEffectsIf context. // Replacing them with a similar instruction potentially enables replacing an instruction @@ -462,7 +462,7 @@ impl Instruction { | IfElse { .. } | ArrayGet { .. } | ArraySet { .. } => { - deduplicate_with_predicate || !self.requires_acir_gen_predicate(dfg) + deduplicate_with_predicate || !self.requires_acir_gen_predicate(&function.dfg) } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 39bda435842..92f3eafb4e6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -159,7 +159,7 @@ impl Function { } context.visited_blocks.insert(block); - context.fold_constants_in_block(&mut self.dfg, &mut dom, block); + context.fold_constants_in_block(self, &mut dom, block); } } } @@ -266,30 +266,31 @@ impl<'brillig> Context<'brillig> { fn fold_constants_in_block( &mut self, - dfg: &mut DataFlowGraph, + function: &mut Function, dom: &mut DominatorTree, block: BasicBlockId, ) { - let instructions = dfg[block].take_instructions(); + let instructions = function.dfg[block].take_instructions(); // Default side effect condition variable with an enabled state. - let mut side_effects_enabled_var = dfg.make_constant(FieldElement::one(), Type::bool()); + let mut side_effects_enabled_var = + function.dfg.make_constant(FieldElement::one(), Type::bool()); for instruction_id in instructions { self.fold_constants_into_instruction( - dfg, + function, dom, block, instruction_id, &mut side_effects_enabled_var, ); } - self.block_queue.extend(dfg[block].successors()); + self.block_queue.extend(function.dfg[block].successors()); } fn fold_constants_into_instruction( &mut self, - dfg: &mut DataFlowGraph, + function: &mut Function, dom: &mut DominatorTree, mut block: BasicBlockId, id: InstructionId, @@ -297,18 +298,23 @@ impl<'brillig> Context<'brillig> { ) { let constraint_simplification_mapping = self.get_constraint_map(*side_effects_enabled_var); - let instruction = - Self::resolve_instruction(id, block, dfg, dom, constraint_simplification_mapping); + let instruction = Self::resolve_instruction( + id, + block, + &function.dfg, + dom, + constraint_simplification_mapping, + ); - let old_results = dfg.instruction_results(id).to_vec(); + let old_results = function.dfg.instruction_results(id).to_vec(); // If a copy of this instruction exists earlier in the block, then reuse the previous results. if let Some(cache_result) = - self.get_cached(dfg, dom, &instruction, *side_effects_enabled_var, block) + self.get_cached(&function.dfg, dom, &instruction, *side_effects_enabled_var, block) { match cache_result { CacheResult::Cached(cached) => { - Self::replace_result_ids(dfg, &old_results, cached); + Self::replace_result_ids(&mut function.dfg, &old_results, cached); return; } CacheResult::NeedToHoistToCommonBlock(dominator) => { @@ -327,7 +333,7 @@ impl<'brillig> Context<'brillig> { &instruction, &old_results, block, - dfg, + &mut function.dfg, self.brillig_info, ) .unwrap_or_else(|| { @@ -337,16 +343,16 @@ impl<'brillig> Context<'brillig> { instruction.clone(), &old_results, block, - dfg, + &mut function.dfg, ) }); - Self::replace_result_ids(dfg, &old_results, &new_results); + Self::replace_result_ids(&mut function.dfg, &old_results, &new_results); self.cache_instruction( instruction.clone(), new_results, - dfg, + function, *side_effects_enabled_var, block, ); @@ -433,7 +439,7 @@ impl<'brillig> Context<'brillig> { &mut self, instruction: Instruction, instruction_results: Vec, - dfg: &DataFlowGraph, + function: &Function, side_effects_enabled_var: ValueId, block: BasicBlockId, ) { @@ -442,11 +448,11 @@ impl<'brillig> Context<'brillig> { // to map from the more complex to the simpler value. if let Instruction::Constrain(lhs, rhs, _) = instruction { // These `ValueId`s should be fully resolved now. - if let Some((complex, simple)) = simplify(dfg, lhs, rhs) { + if let Some((complex, simple)) = simplify(&function.dfg, lhs, rhs) { self.get_constraint_map(side_effects_enabled_var) .entry(complex) .or_default() - .add(dfg, simple, block); + .add(&function.dfg, simple, block); } } } @@ -454,9 +460,9 @@ impl<'brillig> Context<'brillig> { // If the instruction doesn't have side-effects and if it won't interact with enable_side_effects during acir_gen, // we cache the results so we can reuse them if the same instruction appears again later in the block. // Others have side effects representing failure, which are implicit in the ACIR code and can also be deduplicated. - if instruction.can_be_deduplicated(dfg, self.use_constraint_info) { + if instruction.can_be_deduplicated(function, self.use_constraint_info) { let use_predicate = - self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); + self.use_constraint_info && instruction.requires_acir_gen_predicate(&function.dfg); let predicate = use_predicate.then_some(side_effects_enabled_var); self.cached_instruction_results diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 8d3fa9cc615..fc1cc0b15ef 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -127,8 +127,9 @@ impl Context { .push(instructions_len - instruction_index - 1); } } else { - use Instruction::*; - if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + // We can't remove rc instructions if they're loaded from a reference + // since we'd have no way of knowing whether the reference is still used. + if Self::is_inc_dec_instruction_on_known_array(instruction, &function.dfg) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { @@ -140,7 +141,7 @@ impl Context { rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } - self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); + self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays(&function.dfg)); self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, @@ -337,6 +338,28 @@ impl Context { inserted_check } + + /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` + /// operating on an array directly from a `Instruction::MakeArray` or an + /// intrinsic known to return a fresh array. + fn is_inc_dec_instruction_on_known_array( + instruction: &Instruction, + dfg: &DataFlowGraph, + ) -> bool { + use Instruction::*; + if let IncrementRc { value } | DecrementRc { value } = instruction { + if let Value::Instruction { instruction, .. } = &dfg[*value] { + return match &dfg[*instruction] { + MakeArray { .. } => true, + Call { func, .. } => { + matches!(&dfg[*func], Value::Intrinsic(_) | Value::ForeignFunction(_)) + } + _ => false, + }; + } + } + false + } } fn instruction_might_result_in_out_of_bounds( @@ -513,7 +536,7 @@ struct RcTracker { // We also separately track all IncrementRc instructions and all arrays which have been mutably borrowed. // If an array has not been mutably borrowed we can then safely remove all IncrementRc instructions on that array. inc_rcs: HashMap>, - mut_borrowed_arrays: HashSet, + mutated_array_types: HashSet, // The SSA often creates patterns where after simplifications we end up with repeat // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, // and if the current instruction is also an IncrementRc on the same value we remove the current instruction. @@ -567,25 +590,28 @@ impl RcTracker { } } - self.mut_borrowed_arrays.insert(*array); + self.mutated_array_types.insert(typ); } Instruction::Store { value, .. } => { - // We are very conservative and say that any store of an array value means it has the potential - // to be mutated. This is done due to the tracking of mutable borrows still being per block. + // We are very conservative and say that any store of an array value means that any + // array of that type has the potential to be mutated. This is done due to the + // tracking of mutable borrows still being per block and that we don't have the + // aliasing information from mem2reg. let typ = function.dfg.type_of_value(*value); if matches!(&typ, Type::Array(..) | Type::Slice(..)) { - self.mut_borrowed_arrays.insert(*value); + self.mutated_array_types.insert(typ); } } _ => {} } } - fn get_non_mutated_arrays(&self) -> HashSet { + fn get_non_mutated_arrays(&self, dfg: &DataFlowGraph) -> HashSet { self.inc_rcs .keys() .filter_map(|value| { - if !self.mut_borrowed_arrays.contains(value) { + let typ = dfg.type_of_value(*value); + if !self.mutated_array_types.contains(&typ) { Some(&self.inc_rcs[value]) } else { None diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index f1dd511402e..442db4ea31b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -189,8 +189,7 @@ impl<'f> LoopInvariantContext<'f> { !self.defined_in_loop.contains(&value) || self.loop_invariants.contains(&value); }); - let can_be_deduplicated = instruction - .can_be_deduplicated(&self.inserter.function.dfg, false) + let can_be_deduplicated = instruction.can_be_deduplicated(self.inserter.function, false) || self.can_be_deduplicated_from_upper_bound(&instruction); is_loop_invariant && can_be_deduplicated diff --git a/test_programs/execution_success/reference_counts/src/main.nr b/test_programs/execution_success/reference_counts/src/main.nr index 7ab7de893fa..e04e51a33f0 100644 --- a/test_programs/execution_success/reference_counts/src/main.nr +++ b/test_programs/execution_success/reference_counts/src/main.nr @@ -9,19 +9,16 @@ fn main() { fn borrow(array: [Field; 3], rc_before_call: u32) { assert_refcount(array, rc_before_call); - println(array[0]); } fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { assert_refcount(*array, rc_before_call + 0); // Issue! This should be rc_before_call + 1 array[0] = 5; - println(array[0]); } fn copy_mut(mut array: [Field; 3], rc_before_call: u32) { assert_refcount(array, rc_before_call + 0); // Issue! This should be rc_before_call + 1 array[0] = 6; - println(array[0]); } fn assert_refcount(array: [Field; 3], expected: u32) {