Skip to content

fix: modify ref count recursively for nested arrays #9341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
guipublic wants to merge 1 commit into from
Closed

fix: modify ref count recursively for nested arrays #9341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 10 additions & 60 deletions compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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,
Expand Down
103 changes: 103 additions & 0 deletions compiler/noirc_evaluator/src/brillig/brillig_ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -237,6 +239,107 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> 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<F, Registers>, _: 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
Expand Down
Loading