From 90be9bfe2ee9adb20bc17fff53a890ad2b8cee20 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 28 Jul 2025 19:25:21 +0200 Subject: [PATCH] modify ref count recursively for nested arrays --- .../src/brillig/brillig_gen/brillig_block.rs | 70 ++---------- .../noirc_evaluator/src/brillig/brillig_ir.rs | 103 ++++++++++++++++++ 2 files changed, 113 insertions(+), 60 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 4c47494a430..2ae2e20d261 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -918,47 +918,21 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } Instruction::IncrementRc { value } => { let array_or_vector = self.convert_ssa_value(*value, dfg); - let rc_register = self.brillig_context.allocate_register(); - - // RC is always directly pointed by the array/vector pointer - self.brillig_context - .load_instruction(rc_register, array_or_vector.extract_register()); - - // Ensure we're not incrementing from 0 back to 1 - if self.brillig_context.enable_debug_assertions() { - self.assert_rc_neq_zero(rc_register); - } - - self.brillig_context.codegen_usize_op_in_place( - rc_register, - BrilligBinaryOp::Add, - 1, + let array_type = dfg.type_of_value(*value); + self.brillig_context.increment_rc( + &array_type, + array_or_vector.extract_register(), + true, ); - self.brillig_context - .store_instruction(array_or_vector.extract_register(), rc_register); - self.brillig_context.deallocate_register(rc_register); } Instruction::DecrementRc { value } => { let array_or_vector = self.convert_ssa_value(*value, dfg); - let array_register = array_or_vector.extract_register(); - - let rc_register = self.brillig_context.allocate_register(); - self.brillig_context.load_instruction(rc_register, array_register); - - // Check that the refcount isn't already 0 before we decrement. If we allow it to underflow - // and become usize::MAX, and then return to 1, then it will indicate - // an array as mutable when it probably shouldn't be. - if self.brillig_context.enable_debug_assertions() { - self.assert_rc_neq_zero(rc_register); - } - - self.brillig_context.codegen_usize_op_in_place( - rc_register, - BrilligBinaryOp::Sub, - 1, + let array_type = dfg.type_of_value(*value); + self.brillig_context.increment_rc( + &array_type, + array_or_vector.extract_register(), + false, ); - self.brillig_context.store_instruction(array_register, rc_register); - self.brillig_context.deallocate_register(rc_register); } Instruction::EnableSideEffectsIf { .. } => { unreachable!("enable_side_effects not supported by brillig") @@ -1083,30 +1057,6 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { self.brillig_context.set_call_stack(CallStackId::root()); } - /// Debug utility method to determine whether an array's reference count (RC) is zero. - /// If RC's have drifted down to zero it means the RC increment/decrement instructions - /// have been written incorrectly. - /// - /// Should only be called if [BrilligContext::enable_debug_assertions] returns true. - fn assert_rc_neq_zero(&mut self, rc_register: MemoryAddress) { - let zero = SingleAddrVariable::new(self.brillig_context.allocate_register(), 32); - - self.brillig_context.const_instruction(zero, FieldElement::zero()); - - let condition = SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - - self.brillig_context.memory_op_instruction( - zero.address, - rc_register, - condition.address, - BrilligBinaryOp::Equals, - ); - self.brillig_context.not_instruction(condition, condition); - self.brillig_context - .codegen_constrain(condition, Some("array ref-count underflow detected".to_owned())); - self.brillig_context.deallocate_single_addr(condition); - } - /// Internal method to codegen an [Instruction::Call] to a [Value::Function] fn convert_ssa_function_call( &mut self, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index a545cd317e5..0cf0bb7232a 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -30,6 +30,8 @@ pub(crate) use instructions::BrilligBinaryOp; use noirc_errors::call_stack::CallStackId; use registers::{RegisterAllocator, ScratchSpace}; +use crate::ssa::ir::types::Type; + use self::{artifact::BrilligArtifact, debug_show::DebugToString, registers::Stack}; use acvm::{ AcirField, @@ -237,6 +239,107 @@ impl BrilligContext< self.deallocate_single_addr(right_abs_value); self.deallocate_single_addr(result_is_negative); } + + /// Debug utility method to determine whether an array's reference count (RC) is zero. + /// If RC's have drifted down to zero it means the RC increment/decrement instructions + /// have been written incorrectly. + /// + /// Should only be called if [BrilligContext::enable_debug_assertions] returns true. + pub(crate) fn assert_rc_neq_zero(&mut self, rc_register: MemoryAddress) { + let zero = SingleAddrVariable::new(self.allocate_register(), 32); + + self.const_instruction(zero, F::zero()); + + let condition = SingleAddrVariable::new(self.allocate_register(), 1); + + self.memory_op_instruction( + zero.address, + rc_register, + condition.address, + BrilligBinaryOp::Equals, + ); + self.not_instruction(condition, condition); + self.codegen_constrain(condition, Some("array ref-count underflow detected".to_owned())); + self.deallocate_single_addr(condition); + } + + /// Modify the reference counter of array and slices, recursively for nested array + /// THe increment boolean input is true for incrementing the ref count, and false for decrementing + pub(crate) fn increment_rc(&mut self, typ: &Type, pointer: MemoryAddress, increment: bool) { + match typ { + Type::Reference(_) => unreachable!(), + Type::Array(items, len) => { + let register = self.allocate_register(); + // Load the ref count: RC is always directly pointed by the array/vector pointer + self.load_instruction(register, pointer); + + // Ensure we're not incrementing from 0 back to 1, or decrementing 0 + if self.enable_debug_assertions() { + self.assert_rc_neq_zero(register); + } + if increment { + self.codegen_usize_op_in_place(register, BrilligBinaryOp::Add, 1); + } else { + self.codegen_usize_op_in_place(register, BrilligBinaryOp::Sub, 1); + } + + self.store_instruction(pointer, register); + + // Increment array pointer for processing array elements + let iterator = self.allocate_register(); + self.mov_instruction(iterator, pointer); + self.codegen_usize_op_in_place(iterator, BrilligBinaryOp::Add, 1); + + //Increment ref count recursively for each array element + for _ in 0..*len as usize { + for item in items.as_slice() { + self.load_instruction(register, iterator); + self.increment_rc(item, register, increment); + + self.codegen_usize_op_in_place(iterator, BrilligBinaryOp::Add, 1); + } + } + self.deallocate_register(register); + } + Type::Slice(items) => { + let register = self.allocate_register(); + // Load the ref count: RC is always directly pointed by the array/vector pointer + self.load_instruction(register, pointer); + + // Ensure we're not incrementing from 0 back to 1, or decrementing 0 + if self.enable_debug_assertions() { + self.assert_rc_neq_zero(register); + } + + if increment { + self.codegen_usize_op_in_place(register, BrilligBinaryOp::Add, 1); + } else { + self.codegen_usize_op_in_place(register, BrilligBinaryOp::Sub, 1); + } + self.store_instruction(pointer, register); + self.deallocate_register(register); + // Increment array pointer for processing array elements + let iterator = self.allocate_register(); + self.mov_instruction(iterator, pointer); + self.codegen_usize_op_in_place(iterator, BrilligBinaryOp::Add, 1); + let slice_len = self.allocate_register(); + self.load_instruction(slice_len, iterator); + self.codegen_usize_op_in_place(iterator, BrilligBinaryOp::Add, 2); // Length and capacity + + let inner_pointer = self.allocate_register(); + let body = |ctx: &mut BrilligContext, _: SingleAddrVariable| { + for item in items.as_slice() { + ctx.load_instruction(inner_pointer, iterator); + ctx.increment_rc(item, inner_pointer, increment); + ctx.codegen_usize_op_in_place(iterator, BrilligBinaryOp::Add, 1); + } + }; + self.deallocate_register(inner_pointer); + self.codegen_for_loop(None, slice_len, None, body); + } + _ => (), //not an array; nothing to do + } + } } /// Special brillig context to codegen compiler intrinsic shared procedures