From 5e04067583b71384821ff12503ae1131fbe2b114 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 11:57:08 -0600 Subject: [PATCH 01/16] Avoid incrementing reference counts in some cases --- .../src/ssa/function_builder/mod.rs | 23 ++++++++++++------- .../src/ssa/ssa_gen/context.rs | 9 +++++--- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 ++-- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 0479f8da0b7..055074bc01c 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -442,20 +442,25 @@ impl FunctionBuilder { /// within the given value. If the given value is not an array and does not contain /// any arrays, this does nothing. pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) { - self.update_array_reference_count(value, true); + self.update_array_reference_count(value, true, true); } /// Insert instructions to decrement the reference count of any array(s) stored /// within the given value. If the given value is not an array and does not contain /// any arrays, this does nothing. pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) { - self.update_array_reference_count(value, false); + self.update_array_reference_count(value, false, true); } /// Increment or decrement the given value's reference count if it is an array. /// If it is not an array, this does nothing. Note that inc_rc and dec_rc instructions /// are ignored outside of unconstrained code. - fn update_array_reference_count(&mut self, value: ValueId, increment: bool) { + pub(crate) fn update_array_reference_count( + &mut self, + value: ValueId, + increment: bool, + found_ref: bool, + ) { match self.type_of_value(value) { Type::Numeric(_) => (), Type::Function => (), @@ -463,16 +468,18 @@ impl FunctionBuilder { if element.contains_an_array() { let reference = value; let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, increment); + self.update_array_reference_count(value, increment, true); } } Type::Array(..) | Type::Slice(..) => { // If there are nested arrays or slices, we wait until ArrayGet // is issued to increment the count of that array. - if increment { - self.insert_inc_rc(value); - } else { - self.insert_dec_rc(value); + if found_ref { + if increment { + self.insert_inc_rc(value); + } else { + self.insert_dec_rc(value); + } } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 0c6041029da..2b8bd80807d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -174,6 +174,7 @@ impl<'a> FunctionContext<'a> { let element_type = self.builder.current_function.dfg.type_of_value(value_to_store); let alloc = self.builder.insert_allocate(element_type); self.builder.insert_store(alloc, value_to_store); + self.builder.increment_array_reference_count(value_to_store); let typ = self.builder.type_of_value(value_to_store); Value::Mutable(alloc, typ) } @@ -735,7 +736,7 @@ impl<'a> FunctionContext<'a> { // Reference counting in brillig relies on us incrementing reference // counts when arrays/slices are constructed or indexed. // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. - self.builder.increment_array_reference_count(reference); + // self.builder.increment_array_reference_count(reference); self.builder.insert_load(reference, element_type).into() }) } @@ -916,7 +917,9 @@ impl<'a> FunctionContext<'a> { let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); for parameter in parameters { - self.builder.increment_array_reference_count(parameter); + // Use `update_array_reference_count` here to avoid reference counts for + // immutable arrays that aren't behind references. + self.builder.update_array_reference_count(parameter, true, false); } entry @@ -933,7 +936,7 @@ impl<'a> FunctionContext<'a> { dropped_parameters.retain(|parameter| !terminator_args.contains(parameter)); for parameter in dropped_parameters { - self.builder.decrement_array_reference_count(parameter); + self.builder.update_array_reference_count(parameter, false, false); } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index c50f0a7f45c..d28236bd360 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -665,12 +665,11 @@ impl<'a> FunctionContext<'a> { values = values.map(|value| { let value = value.eval(self); - // Make sure to increment array reference counts on each let binding - self.builder.increment_array_reference_count(value); - Tree::Leaf(if let_expr.mutable { self.new_mutable_variable(value) } else { + // `new_mutable_variable` already increments rcs internally + self.builder.increment_array_reference_count(value); value::Value::Normal(value) }) }); From d2a7849885ef27798d4c14246174c8537fc32f56 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 12:52:33 -0600 Subject: [PATCH 02/16] Move inc_rc 2 instructions earlier --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 2b8bd80807d..33f0477be87 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -172,9 +172,9 @@ impl<'a> FunctionContext<'a> { /// Always returns a Value::Mutable wrapping the allocate instruction. pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { let element_type = self.builder.current_function.dfg.type_of_value(value_to_store); + self.builder.increment_array_reference_count(value_to_store); let alloc = self.builder.insert_allocate(element_type); self.builder.insert_store(alloc, value_to_store); - self.builder.increment_array_reference_count(value_to_store); let typ = self.builder.type_of_value(value_to_store); Value::Mutable(alloc, typ) } From 65054d6a400e9f097cadd8597d83a6603534d7b5 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 13:47:26 -0600 Subject: [PATCH 03/16] Trigger CI From 97cf6748a7c10df01d2777f0e513f68d0742f2e2 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 13:54:39 -0600 Subject: [PATCH 04/16] Trigger CI, remove external repos tag since noir-edwards needs an update From 4c234cfbc76a4a1c59511e1cc569af01abb3b09e Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 20 Nov 2024 15:01:19 -0600 Subject: [PATCH 05/16] Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 33f0477be87..eba3865cdb8 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -736,7 +736,6 @@ impl<'a> FunctionContext<'a> { // Reference counting in brillig relies on us incrementing reference // counts when arrays/slices are constructed or indexed. // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. - // self.builder.increment_array_reference_count(reference); self.builder.insert_load(reference, element_type).into() }) } From b4f01873928255d0c9ac3c7892d2cd38c6ce35ff Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 07:42:44 -0600 Subject: [PATCH 06/16] Remove need for extra parameter --- .../src/ssa/function_builder/mod.rs | 23 +++++++------------ .../src/ssa/ssa_gen/context.rs | 11 +++++---- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 055074bc01c..0479f8da0b7 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -442,25 +442,20 @@ impl FunctionBuilder { /// within the given value. If the given value is not an array and does not contain /// any arrays, this does nothing. pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) { - self.update_array_reference_count(value, true, true); + self.update_array_reference_count(value, true); } /// Insert instructions to decrement the reference count of any array(s) stored /// within the given value. If the given value is not an array and does not contain /// any arrays, this does nothing. pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) { - self.update_array_reference_count(value, false, true); + self.update_array_reference_count(value, false); } /// Increment or decrement the given value's reference count if it is an array. /// If it is not an array, this does nothing. Note that inc_rc and dec_rc instructions /// are ignored outside of unconstrained code. - pub(crate) fn update_array_reference_count( - &mut self, - value: ValueId, - increment: bool, - found_ref: bool, - ) { + fn update_array_reference_count(&mut self, value: ValueId, increment: bool) { match self.type_of_value(value) { Type::Numeric(_) => (), Type::Function => (), @@ -468,18 +463,16 @@ impl FunctionBuilder { if element.contains_an_array() { let reference = value; let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, increment, true); + self.update_array_reference_count(value, increment); } } Type::Array(..) | Type::Slice(..) => { // If there are nested arrays or slices, we wait until ArrayGet // is issued to increment the count of that array. - if found_ref { - if increment { - self.insert_inc_rc(value); - } else { - self.insert_dec_rc(value); - } + if increment { + self.insert_inc_rc(value); + } else { + self.insert_dec_rc(value); } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index eba3865cdb8..ddc3365b551 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -916,9 +916,10 @@ impl<'a> FunctionContext<'a> { let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); for parameter in parameters { - // Use `update_array_reference_count` here to avoid reference counts for - // immutable arrays that aren't behind references. - self.builder.update_array_reference_count(parameter, true, false); + // Avoid reference counts for immutable arrays that aren't behind references. + if self.builder.current_function.dfg.value_is_reference(parameter) { + self.builder.increment_array_reference_count(parameter); + } } entry @@ -935,7 +936,9 @@ impl<'a> FunctionContext<'a> { dropped_parameters.retain(|parameter| !terminator_args.contains(parameter)); for parameter in dropped_parameters { - self.builder.update_array_reference_count(parameter, false, false); + if self.builder.current_function.dfg.value_is_reference(parameter) { + self.builder.decrement_array_reference_count(parameter); + } } } From 204de4ae31555d7fc260901cd36e5c612ba3bf4a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 08:03:53 -0600 Subject: [PATCH 07/16] Start rc at 0, not 1 --- .../src/brillig/brillig_ir/codegen_memory.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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..d9b91d85018 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` @@ -424,7 +426,7 @@ impl BrilligContext< self.indirect_const_instruction( array.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), + INITIAL_ARRAY_REF_COUNT.into(), ); } @@ -438,7 +440,7 @@ impl BrilligContext< self.indirect_const_instruction( vector.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), + INITIAL_ARRAY_REF_COUNT.into(), ); // Write size @@ -498,7 +500,7 @@ impl BrilligContext< self.indirect_const_instruction( vector.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), + INITIAL_ARRAY_REF_COUNT.into(), ); // Initialize size From 1e121883f8438e22f4dc7a8aa01c3832ebf0c693 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Thu, 21 Nov 2024 14:48:00 +0000 Subject: [PATCH 08/16] added argument for initial rc --- .../src/brillig/brillig_gen/brillig_block.rs | 8 +++++--- .../src/brillig/brillig_ir/codegen_intrinsic.rs | 2 +- .../src/brillig/brillig_ir/codegen_memory.rs | 14 ++++++++++---- .../src/brillig/brillig_ir/entry_point.rs | 4 ++-- .../brillig_ir/procedures/prepare_vector_push.rs | 2 ++ .../brillig_ir/procedures/vector_pop_back.rs | 2 +- .../brillig_ir/procedures/vector_pop_front.rs | 3 ++- .../brillig/brillig_ir/procedures/vector_remove.rs | 2 +- 8 files changed, 24 insertions(+), 13 deletions(-) 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 36e1ee90e11..7dfca27bc25 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -492,6 +492,7 @@ impl<'block> BrilligBlock<'block> { destination_vector, source_size_register, None, + None, ); // Items @@ -772,13 +773,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!( @@ -1773,7 +1775,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 d9b91d85018..130f5906232 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs @@ -421,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, - INITIAL_ARRAY_REF_COUNT.into(), + initial_rc.unwrap_or(INITIAL_ARRAY_REF_COUNT).into(), ); } @@ -435,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, - INITIAL_ARRAY_REF_COUNT.into(), + initial_rc.unwrap_or(INITIAL_ARRAY_REF_COUNT).into(), ); // Write size @@ -461,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) @@ -473,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 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 From 6a4be880bdd1d8dd8a0f9850e372882b4620fb82 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 11:52:12 -0600 Subject: [PATCH 09/16] Don't remove necessary RC instructions --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 8d3fa9cc615..0460b29753a 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| { @@ -337,6 +338,21 @@ impl Context { inserted_check } + + /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` + /// operating on an array directly from a `Instruction::MakeArray`. + 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 matches!(&dfg[*instruction], MakeArray { .. }); + } + } + false + } } fn instruction_might_result_in_out_of_bounds( From a750a7aba3fa8dfea0d9e30b61a2140cf3baf41e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 12:01:14 -0600 Subject: [PATCH 10/16] Disable rc_tracker --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 0460b29753a..584e852e01b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,7 +108,7 @@ impl Context { let instructions_len = block.instructions().len(); - let mut rc_tracker = RcTracker::default(); + // let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -138,11 +138,11 @@ impl Context { } } - rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); + // 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.rc_pairs_to_remove); + // self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); + // self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those From 6543754566177a5c441dcb912893d8a660e51d0b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 10:26:15 -0600 Subject: [PATCH 11/16] Narrow down issue to non_mutated_arrays tracking --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 584e852e01b..3d1be5578ae 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,7 +108,7 @@ impl Context { let instructions_len = block.instructions().len(); - // let mut rc_tracker = RcTracker::default(); + let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -138,11 +138,11 @@ impl Context { } } - // rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); + 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.rc_pairs_to_remove); + self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those From 4b3950ad88a0fa4836ed4999d9c08b7652b98a79 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 13:17:33 -0600 Subject: [PATCH 12/16] Disable test --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 58 ++++++++++----------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 3d1be5578ae..b3bd34a3199 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -845,33 +845,33 @@ mod test { assert!(matches!(&main.dfg[instructions[3]], Instruction::ArrayGet { .. })); } - #[test] - fn remove_inc_rcs_that_are_never_mutably_borrowed() { - let src = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - inc_rc v0 - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v2 - } - "; - let ssa = Ssa::from_str(src).unwrap(); - let main = ssa.main(); - - // The instruction count never includes the terminator instruction - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); - - let expected = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - v2 = array_get v0, index u32 0 -> Field - return v2 - } - "; - let ssa = ssa.dead_instruction_elimination(); - assert_normalized_ssa_equals(ssa, expected); - } + // #[test] + // fn remove_inc_rcs_that_are_never_mutably_borrowed() { + // let src = " + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // inc_rc v0 + // inc_rc v0 + // inc_rc v0 + // v2 = array_get v0, index u32 0 -> Field + // inc_rc v0 + // return v2 + // } + // "; + // let ssa = Ssa::from_str(src).unwrap(); + // let main = ssa.main(); + + // // The instruction count never includes the terminator instruction + // assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); + + // let expected = " + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // v2 = array_get v0, index u32 0 -> Field + // return v2 + // } + // "; + // let ssa = ssa.dead_instruction_elimination(); + // assert_normalized_ssa_equals(ssa, expected); + // } } From 602e1e6b8e1be02ea9d6b25a22e7dd3426ed1471 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 14:43:48 -0600 Subject: [PATCH 13/16] Reintroduce optimization; it is per type now --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 77 +++++++++++---------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index b3bd34a3199..ce8577f07dd 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -141,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, @@ -529,7 +529,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. @@ -583,25 +583,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 @@ -845,33 +848,33 @@ mod test { assert!(matches!(&main.dfg[instructions[3]], Instruction::ArrayGet { .. })); } - // #[test] - // fn remove_inc_rcs_that_are_never_mutably_borrowed() { - // let src = " - // acir(inline) fn main f0 { - // b0(v0: [Field; 2]): - // inc_rc v0 - // inc_rc v0 - // inc_rc v0 - // v2 = array_get v0, index u32 0 -> Field - // inc_rc v0 - // return v2 - // } - // "; - // let ssa = Ssa::from_str(src).unwrap(); - // let main = ssa.main(); - - // // The instruction count never includes the terminator instruction - // assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); - - // let expected = " - // acir(inline) fn main f0 { - // b0(v0: [Field; 2]): - // v2 = array_get v0, index u32 0 -> Field - // return v2 - // } - // "; - // let ssa = ssa.dead_instruction_elimination(); - // assert_normalized_ssa_equals(ssa, expected); - // } + #[test] + fn remove_inc_rcs_that_are_never_mutably_borrowed() { + let src = " + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + inc_rc v0 + inc_rc v0 + inc_rc v0 + v2 = array_get v0, index u32 0 -> Field + inc_rc v0 + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main(); + + // The instruction count never includes the terminator instruction + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); + + let expected = " + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + v2 = array_get v0, index u32 0 -> Field + return v2 + } + "; + let ssa = ssa.dead_instruction_elimination(); + assert_normalized_ssa_equals(ssa, expected); + } } From fdc8dd07e49f5c645ec4d6699d8e83df00954f57 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 14:57:16 -0600 Subject: [PATCH 14/16] Consider arrays returned by intrinsics to be fresh as well --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index ce8577f07dd..fc1cc0b15ef 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -340,7 +340,8 @@ impl Context { } /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` - /// operating on an array directly from a `Instruction::MakeArray`. + /// 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, @@ -348,7 +349,13 @@ impl Context { use Instruction::*; if let IncrementRc { value } | DecrementRc { value } = instruction { if let Value::Instruction { instruction, .. } = &dfg[*value] { - return matches!(&dfg[*instruction], MakeArray { .. }); + return match &dfg[*instruction] { + MakeArray { .. } => true, + Call { func, .. } => { + matches!(&dfg[*func], Value::Intrinsic(_) | Value::ForeignFunction(_)) + } + _ => false, + }; } } false From b36d8d3a6d57e9e8dc2fde06b189100bdf0014d3 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 2 Dec 2024 12:02:21 -0600 Subject: [PATCH 15/16] Don't dedup arrays in brillig --- .../src/ssa/ir/function_inserter.rs | 3 +- .../noirc_evaluator/src/ssa/ir/instruction.rs | 10 +++---- .../src/ssa/opt/constant_folding.rs | 29 ++++++++++--------- .../src/ssa/opt/loop_invariant.rs | 2 +- 4 files changed, 23 insertions(+), 21 deletions(-) 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 9958aadd443..ee4a66c7a9b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -333,7 +333,7 @@ impl Instruction { /// rely on predicates can be deduplicated as well. pub(crate) fn can_be_deduplicated( &self, - dfg: &DataFlowGraph, + function: &Function, deduplicate_with_predicate: bool, ) -> bool { use Instruction::*; @@ -347,7 +347,7 @@ impl Instruction { | IncrementRc { .. } | DecrementRc { .. } => false, - Call { func, .. } => match dfg[*func] { + Call { func, .. } => match function.dfg[*func] { Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), _ => false, }, @@ -355,8 +355,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 @@ -369,7 +369,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 019bace33a3..454f0580670 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -228,7 +228,7 @@ impl<'brillig> Context<'brillig> { for instruction_id in instructions { self.fold_constants_into_instruction( - &mut function.dfg, + function, block, instruction_id, &mut side_effects_enabled_var, @@ -239,22 +239,23 @@ impl<'brillig> Context<'brillig> { fn fold_constants_into_instruction( &mut self, - dfg: &mut DataFlowGraph, + function: &mut Function, mut block: BasicBlockId, id: InstructionId, side_effects_enabled_var: &mut ValueId, ) { let constraint_simplification_mapping = self.get_constraint_map(*side_effects_enabled_var); - let instruction = Self::resolve_instruction(id, dfg, constraint_simplification_mapping); - let old_results = dfg.instruction_results(id).to_vec(); + let instruction = + Self::resolve_instruction(id, &function.dfg, constraint_simplification_mapping); + 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, &instruction, *side_effects_enabled_var, block) + self.get_cached(&function.dfg, &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, _cached) => { @@ -273,7 +274,7 @@ impl<'brillig> Context<'brillig> { &instruction, &old_results, block, - dfg, + &mut function.dfg, self.brillig_info, ) .unwrap_or_else(|| { @@ -283,16 +284,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, ); @@ -368,7 +369,7 @@ impl<'brillig> Context<'brillig> { &mut self, instruction: Instruction, instruction_results: Vec, - dfg: &DataFlowGraph, + function: &Function, side_effects_enabled_var: ValueId, block: BasicBlockId, ) { @@ -377,7 +378,7 @@ 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. - match (&dfg[lhs], &dfg[rhs]) { + match (&function.dfg[lhs], &function.dfg[rhs]) { // Ignore trivial constraints (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => (), @@ -403,9 +404,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. - 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/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 14233ca73e5..6fe84ffcc3b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -143,7 +143,7 @@ impl<'f> LoopInvariantContext<'f> { is_loop_invariant &= !self.defined_in_loop.contains(&value) || self.loop_invariants.contains(&value); }); - is_loop_invariant && instruction.can_be_deduplicated(&self.inserter.function.dfg, false) + is_loop_invariant && instruction.can_be_deduplicated(self.inserter.function, false) } fn map_dependent_instructions(&mut self) { From c8b0ec9a624375ab8c1ce7d1d8de47c4502a0c05 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 2 Dec 2024 12:06:04 -0600 Subject: [PATCH 16/16] Remove printlns --- test_programs/execution_success/reference_counts/src/main.nr | 3 --- 1 file changed, 3 deletions(-) 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) {