From 5e04067583b71384821ff12503ae1131fbe2b114 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 11:57:08 -0600 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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); + } } }