From 5e04067583b71384821ff12503ae1131fbe2b114 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 11:57:08 -0600 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] Trigger CI, remove external repos tag since noir-edwards needs an update From 5a82c25a4c38f1d9426ad7862f35f2adcffe97f7 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 20 Nov 2024 14:42:16 -0600 Subject: [PATCH 5/5] Actually just dont inc param rcs at all --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 33f0477be87..a22b3847e85 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -929,7 +929,7 @@ impl<'a> FunctionContext<'a> { /// This will issue DecrementRc instructions for any arrays in the given starting scope /// block's parameters. Arrays that are also used in terminator instructions for the scope are /// ignored. - pub(crate) fn end_scope(&mut self, scope: BasicBlockId, terminator_args: &[ValueId]) { + pub(crate) fn end_function(&mut self, scope: BasicBlockId, terminator_args: &[ValueId]) { let mut dropped_parameters = self.builder.current_function.dfg.block_parameters(scope).to_vec(); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d28236bd360..8a3ffa4420f 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -125,10 +125,10 @@ impl<'a> FunctionContext<'a> { /// Codegen a function's body and set its return value to that of its last parameter. /// For functions returning nothing, this will be an empty list. fn codegen_function_body(&mut self, body: &Expression) -> Result<(), RuntimeError> { - let entry_block = self.increment_parameter_rcs(); + // let entry_block = self.builder.current_function.entry_block(); // self.increment_parameter_rcs(); let return_value = self.codegen_expression(body)?; let results = return_value.into_value_list(self); - self.end_scope(entry_block, &results); + // self.end_function(entry_block, &results); self.builder.terminate_with_return(results); Ok(())