From d8710c4442be2fcffc348f1f5776bc278d028ad0 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 13 Mar 2024 15:17:41 -0500 Subject: [PATCH] chore: Add `Instruction::DecrementRc` (#4525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/4522 ## Summary\* Experimenting with this to see how much it improves performance of unconstrained code using arrays. ## Additional Context Currently the new dec_rc instruction is only issued for function parameters when a function is finished - assuming the parameters are not also returned. CC @sirasistant for visibility ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Álvaro Rodríguez --- .../src/brillig/brillig_gen/brillig_block.rs | 18 ++++++++--- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- .../src/ssa/function_builder/mod.rs | 21 ++++++++++++- .../noirc_evaluator/src/ssa/ir/instruction.rs | 16 +++++++++- .../noirc_evaluator/src/ssa/ir/printer.rs | 3 ++ compiler/noirc_evaluator/src/ssa/opt/die.rs | 26 +++++++++------- .../src/ssa/ssa_gen/context.rs | 31 +++++++++++++++++++ .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 9 +++--- 8 files changed, 103 insertions(+), 23 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 911f4c1924e..938a80e87d1 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -227,9 +227,7 @@ impl<'block> BrilligBlock<'block> { dfg, ); } - _ => { - todo!("ICE: Param type not supported") - } + Type::Function => todo!("ICE: Type::Function Param not supported"), } } } @@ -661,11 +659,21 @@ impl<'block> BrilligBlock<'block> { let rc_register = match self.convert_ssa_value(*value, dfg) { BrilligVariable::BrilligArray(BrilligArray { rc, .. }) | BrilligVariable::BrilligVector(BrilligVector { rc, .. }) => rc, - _ => unreachable!("ICE: increment rc on non-array"), + other => unreachable!("ICE: increment rc on non-array: {other:?}"), }; self.brillig_context.usize_op_in_place(rc_register, BinaryIntOp::Add, 1); } - _ => todo!("ICE: Instruction not supported {instruction:?}"), + Instruction::DecrementRc { value } => { + let rc_register = match self.convert_ssa_value(*value, dfg) { + BrilligVariable::BrilligArray(BrilligArray { rc, .. }) + | BrilligVariable::BrilligVector(BrilligVector { rc, .. }) => rc, + other => unreachable!("ICE: decrement rc on non-array: {other:?}"), + }; + self.brillig_context.usize_op_in_place(rc_register, BinaryIntOp::Sub, 1); + } + Instruction::EnableSideEffects { .. } => { + todo!("enable_side_effects not supported by brillig") + } }; let dead_variables = self diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 140ed0b53ff..4442efe286a 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -503,7 +503,7 @@ impl Context { Instruction::Load { .. } => { unreachable!("Expected all load instructions to be removed before acir_gen") } - Instruction::IncrementRc { .. } => { + Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => { // Do nothing. Only Brillig needs to worry about reference counted arrays } Instruction::RangeCheck { value, max_bit_size, assert_message } => { diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index bf34a47485b..2c39c83b342 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -384,6 +384,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); + } + + /// 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); + } + + /// 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) { match self.type_of_value(value) { Type::Numeric(_) => (), Type::Function => (), @@ -396,7 +410,12 @@ impl FunctionBuilder { typ @ Type::Array(..) | typ @ Type::Slice(..) => { // If there are nested arrays or slices, we wait until ArrayGet // is issued to increment the count of that array. - self.insert_instruction(Instruction::IncrementRc { value }, None); + let instruction = if increment { + Instruction::IncrementRc { value } + } else { + Instruction::DecrementRc { value } + }; + self.insert_instruction(instruction, None); // This is a bit odd, but in brillig the inc_rc instruction operates on // a copy of the array's metadata, so we need to re-store a loaded array diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 0b6c7074e45..afade4b0616 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -194,6 +194,13 @@ pub(crate) enum Instruction { /// implemented via reference counting. In ACIR code this is done with im::Vector and these /// IncrementRc instructions are ignored. IncrementRc { value: ValueId }, + + /// An instruction to decrement the reference count of a value. + /// + /// This currently only has an effect in Brillig code where array sharing and copy on write is + /// implemented via reference counting. In ACIR code this is done with im::Vector and these + /// DecrementRc instructions are ignored. + DecrementRc { value: ValueId }, } impl Instruction { @@ -214,6 +221,7 @@ impl Instruction { Instruction::Constrain(..) | Instruction::Store { .. } | Instruction::IncrementRc { .. } + | Instruction::DecrementRc { .. } | Instruction::RangeCheck { .. } | Instruction::EnableSideEffects { .. } => InstructionResultType::None, Instruction::Allocate { .. } @@ -250,6 +258,7 @@ impl Instruction { | Load { .. } | Store { .. } | IncrementRc { .. } + | DecrementRc { .. } | RangeCheck { .. } => false, Call { func, .. } => match dfg[*func] { @@ -285,6 +294,7 @@ impl Instruction { | Store { .. } | EnableSideEffects { .. } | IncrementRc { .. } + | DecrementRc { .. } | RangeCheck { .. } => true, // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. @@ -353,6 +363,7 @@ impl Instruction { Instruction::ArraySet { array: f(*array), index: f(*index), value: f(*value) } } Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) }, + Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) }, Instruction::RangeCheck { value, max_bit_size, assert_message } => { Instruction::RangeCheck { value: f(*value), @@ -409,7 +420,9 @@ impl Instruction { Instruction::EnableSideEffects { condition } => { f(*condition); } - Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => { + Instruction::IncrementRc { value } + | Instruction::DecrementRc { value } + | Instruction::RangeCheck { value, .. } => { f(*value); } } @@ -554,6 +567,7 @@ impl Instruction { Instruction::Load { .. } => None, Instruction::Store { .. } => None, Instruction::IncrementRc { .. } => None, + Instruction::DecrementRc { .. } => None, Instruction::RangeCheck { value, max_bit_size, .. } => { if let Some(numeric_constant) = dfg.get_numeric_constant(*value) { if numeric_constant.num_bits() < *max_bit_size { diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 9bd43fab1ff..6ef618fba6f 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -188,6 +188,9 @@ fn display_instruction_inner( Instruction::IncrementRc { value } => { writeln!(f, "inc_rc {}", show(*value)) } + Instruction::DecrementRc { value } => { + writeln!(f, "dec_rc {}", show(*value)) + } Instruction::RangeCheck { value, max_bit_size, .. } => { writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index f7d8adb5275..4c7beff0fbe 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -44,7 +44,7 @@ fn dead_instruction_elimination(function: &mut Function) { context.remove_unused_instructions_in_block(function, *block); } - context.remove_increment_rc_instructions(&mut function.dfg); + context.remove_rc_instructions(&mut function.dfg); } /// Per function context for tracking unused values and which instructions to remove. @@ -53,10 +53,10 @@ struct Context { used_values: HashSet, instructions_to_remove: HashSet, - /// IncrementRc instructions must be revisited after the main DIE pass since + /// IncrementRc & DecrementRc instructions must be revisited after the main DIE pass since /// they technically contain side-effects but we still want to remove them if their /// `value` parameter is not used elsewhere. - increment_rc_instructions: Vec<(InstructionId, BasicBlockId)>, + rc_instructions: Vec<(InstructionId, BasicBlockId)>, } impl Context { @@ -85,8 +85,9 @@ impl Context { } else { let instruction = &function.dfg[*instruction_id]; - if let Instruction::IncrementRc { .. } = instruction { - self.increment_rc_instructions.push((*instruction_id, block_id)); + use Instruction::*; + if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { self.mark_used_instruction_results(&function.dfg, value); @@ -145,16 +146,19 @@ impl Context { } } - fn remove_increment_rc_instructions(self, dfg: &mut DataFlowGraph) { - for (increment_rc, block) in self.increment_rc_instructions { - let value = match &dfg[increment_rc] { + fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) { + for (rc, block) in self.rc_instructions { + let value = match &dfg[rc] { Instruction::IncrementRc { value } => *value, - other => unreachable!("Expected IncrementRc instruction, found {other:?}"), + Instruction::DecrementRc { value } => *value, + other => { + unreachable!("Expected IncrementRc or DecrementRc instruction, found {other:?}") + } }; - // This could be more efficient if we have to remove multiple IncrementRcs in a single block + // This could be more efficient if we have to remove multiple instructions in a single block if !self.used_values.contains(&value) { - dfg[block].instructions_mut().retain(|instruction| *instruction != increment_rc); + dfg[block].instructions_mut().retain(|instruction| *instruction != rc); } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 9c760c013a9..409b99958a9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -10,6 +10,7 @@ use noirc_frontend::{BinaryOpKind, Signedness}; use crate::errors::RuntimeError; use crate::ssa::function_builder::FunctionBuilder; +use crate::ssa::ir::basic_block::BasicBlockId; use crate::ssa::ir::dfg::DataFlowGraph; use crate::ssa::ir::function::FunctionId as IrFunctionId; use crate::ssa::ir::function::{Function, RuntimeType}; @@ -1022,6 +1023,36 @@ impl<'a> FunctionContext<'a> { } } } + + /// Increments the reference count of all parameters. Returns the entry block of the function. + /// + /// This is done on parameters rather than call arguments so that we can optimize out + /// paired inc/dec instructions within brillig functions more easily. + pub(crate) fn increment_parameter_rcs(&mut self) -> BasicBlockId { + let entry = self.builder.current_function.entry_block(); + let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); + + for parameter in parameters { + self.builder.increment_array_reference_count(parameter); + } + + entry + } + + /// Ends a local scope of a function. + /// 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]) { + let mut dropped_parameters = + self.builder.current_function.dfg.block_parameters(scope).to_vec(); + + dropped_parameters.retain(|parameter| !terminator_args.contains(parameter)); + + for parameter in dropped_parameters { + self.builder.decrement_array_reference_count(parameter); + } + } } /// True if the given operator cannot be encoded directly and needs diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index f3fa5d1d2f8..3d8ae0bb3eb 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -121,8 +121,11 @@ 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 return_value = self.codegen_expression(body)?; let results = return_value.into_value_list(self); + self.end_scope(entry_block, &results); + self.builder.terminate_with_return(results); Ok(()) } @@ -595,10 +598,8 @@ impl<'a> FunctionContext<'a> { arguments.append(&mut values); } - // If an array is passed as an argument we increase its reference count - for argument in &arguments { - self.builder.increment_array_reference_count(*argument); - } + // Don't need to increment array reference counts when passed in as arguments + // since it is done within the function to each parameter already. self.codegen_intrinsic_call_checks(function, &arguments, call.location); Ok(self.insert_call(function, arguments, &call.return_type, call.location))