Skip to content
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

fix!: Only decrement the counter of an array if its address has not changed #7297

Merged
merged 21 commits into from
Feb 13, 2025
Merged

fix!: Only decrement the counter of an array if its address has not changed #7297

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5d4c429
Add Brillig check that after decrease RC it's at least 1
aakoshh Feb 5, 2025
4652543
Only increment decrement RC if the array pointer has not changed
aakoshh Feb 6, 2025
6582885
Fix clippy
aakoshh Feb 6, 2025
6777c79
Fix parsing
aakoshh Feb 6, 2025
45b939e
Put the assertion of the RC being >= 1 behind an option
aakoshh Feb 7, 2025
f340386
Enable brillig debug assertions in integration tests
aakoshh Feb 7, 2025
cdd0dfa
Merge branch 'master' into af/decrease-rc-to-zero
aakoshh Feb 7, 2025
548987e
Add regression test that shows there is a bug
aakoshh Feb 7, 2025
8520199
Re-enable the fix
aakoshh Feb 7, 2025
19ed727
Merge branch 'af/decrease-rc-to-zero' of github.com:noir-lang/noir in…
aakoshh Feb 7, 2025
20581d6
Mark the regression as unconstrained
aakoshh Feb 7, 2025
5f7606f
Format Noir test
aakoshh Feb 7, 2025
b426024
Merge remote-tracking branch 'origin/master' into af/decrease-rc-to-zero
aakoshh Feb 11, 2025
2796c31
Add BrilligOptions to Context
aakoshh Feb 13, 2025
81ab844
Add Brillig to Context
aakoshh Feb 13, 2025
f19a26f
Pass BrilligOptions to call generation
aakoshh Feb 13, 2025
e1b35d7
Clarify comments in the test
aakoshh Feb 13, 2025
7123b8d
Explain redundant assertions.
aakoshh Feb 13, 2025
36cad20
Use SSA parsing in RC tests
aakoshh Feb 13, 2025
acc6095
Enable src.parse() syntax for SSA
aakoshh Feb 13, 2025
b4750e3
Merge branch 'master' into af/decrease-rc-to-zero
aakoshh Feb 13, 2025
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
10 changes: 9 additions & 1 deletion compiler/noirc_driver/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use fm::{FileId, FileManager};
use iter_extended::vecmap;
use noirc_abi::{AbiParameter, AbiType, AbiValue};
use noirc_errors::{CustomDiagnostic, DiagnosticKind, FileDiagnostic};
use noirc_evaluator::brillig::BrilligOptions;
use noirc_evaluator::create_program;
use noirc_evaluator::errors::RuntimeError;
use noirc_evaluator::ssa::{SsaLogging, SsaProgramArtifact};
Expand Down Expand Up @@ -141,6 +142,10 @@ pub struct CompileOptions {
#[arg(long)]
pub enable_brillig_constraints_check: bool,

/// Flag to turn on extra Brillig bytecode to be generated to guard against invalid states in testing.
#[arg(long, hide = true)]
pub enable_brillig_debug_assertions: bool,

/// Hidden Brillig call check flag to maintain CI compatibility (currently ignored)
#[arg(long, hide = true)]
pub skip_brillig_constraints_check: bool,
Expand Down Expand Up @@ -674,7 +679,10 @@ pub fn compile_no_check(
}
}
},
enable_brillig_logging: options.show_brillig,
brillig_options: BrilligOptions {
enable_debug_trace: options.show_brillig,
enable_debug_assertions: options.enable_brillig_debug_assertions,
},
print_codegen_timings: options.benchmark_codegen,
expression_width: if options.bounded_codegen {
options.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH)
Expand Down
49 changes: 29 additions & 20 deletions compiler/noirc_evaluator/src/acir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ mod brillig_directive;
mod generated_acir;

use crate::brillig::brillig_gen::gen_brillig_for;
use crate::brillig::BrilligOptions;
use crate::brillig::{
brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext,
brillig_ir::artifact::{BrilligParameter, GeneratedBrillig},
Expand Down Expand Up @@ -305,6 +306,7 @@ impl Ssa {
pub(crate) fn into_acir(
self,
brillig: &Brillig,
brillig_options: &BrilligOptions,
expression_width: ExpressionWidth,
) -> Result<Artifacts, RuntimeError> {
let mut acirs = Vec::new();
Expand All @@ -314,7 +316,7 @@ impl Ssa {
for function in self.functions.values() {
let context = Context::new(&mut shared_context, expression_width);
if let Some(mut generated_acir) =
context.convert_ssa_function(&self, function, brillig)?
context.convert_ssa_function(&self, function, brillig, brillig_options)?
{
// We want to be able to insert Brillig stdlib functions anywhere during the ACIR generation process (e.g. such as on the `GeneratedAcir`).
// As we don't want a reference to the `SharedContext` on the generated ACIR itself,
Expand Down Expand Up @@ -388,6 +390,7 @@ impl<'a> Context<'a> {
ssa: &Ssa,
function: &Function,
brillig: &Brillig,
brillig_options: &BrilligOptions,
) -> Result<Option<GeneratedAcir<FieldElement>>, RuntimeError> {
match function.runtime() {
RuntimeType::Acir(inline_type) => {
Expand All @@ -407,7 +410,7 @@ impl<'a> Context<'a> {
}
RuntimeType::Brillig(_) => {
if function.id() == ssa.main_id {
Ok(Some(self.convert_brillig_main(function, brillig)?))
Ok(Some(self.convert_brillig_main(function, brillig, brillig_options)?))
} else {
Ok(None)
}
Expand Down Expand Up @@ -505,6 +508,7 @@ impl<'a> Context<'a> {
mut self,
main_func: &Function,
brillig: &Brillig,
options: &BrilligOptions,
) -> Result<GeneratedAcir<FieldElement>, RuntimeError> {
let dfg = &main_func.dfg;

Expand All @@ -519,7 +523,7 @@ impl<'a> Context<'a> {
let outputs: Vec<AcirType> =
vecmap(main_func.returns(), |result_id| dfg.type_of_value(*result_id).into());

let code = gen_brillig_for(main_func, arguments.clone(), brillig)?;
let code = gen_brillig_for(main_func, arguments.clone(), brillig, options)?;

// We specifically do not attempt execution of the brillig code being generated as this can result in it being
// replaced with constraints on witnesses to the program outputs.
Expand Down Expand Up @@ -927,7 +931,12 @@ impl<'a> Context<'a> {
None,
)?
} else {
let code = gen_brillig_for(func, arguments.clone(), brillig)?;
let code = gen_brillig_for(
func,
arguments.clone(),
brillig,
&BrilligOptions::default(),
)?;
let generated_pointer =
self.shared_context.new_generated_pointer();
let output_values = self.acir_context.brillig_call(
Expand Down Expand Up @@ -2904,7 +2913,7 @@ mod test {

use crate::{
acir::BrilligStdlibFunc,
brillig::Brillig,
brillig::{Brillig, BrilligOptions},
ssa::{
function_builder::FunctionBuilder,
ir::{
Expand Down Expand Up @@ -3005,7 +3014,7 @@ mod test {
let ssa = builder.finish().generate_entry_point_index();

let (acir_functions, _, _, _) = ssa
.into_acir(&Brillig::default(), ExpressionWidth::default())
.into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");
// Expected result:
// main f0
Expand Down Expand Up @@ -3111,7 +3120,7 @@ mod test {

let (acir_functions, _, _, _) = ssa
.generate_entry_point_index()
.into_acir(&Brillig::default(), ExpressionWidth::default())
.into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");
// The expected result should look very similar to the above test expect that the input witnesses of the `Call`
// opcodes will be different. The changes can discerned from the checks below.
Expand Down Expand Up @@ -3215,7 +3224,7 @@ mod test {
let ssa = builder.finish().generate_entry_point_index();

let (acir_functions, _, _, _) = ssa
.into_acir(&Brillig::default(), ExpressionWidth::default())
.into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 3, "Should have three ACIR functions");
Expand Down Expand Up @@ -3336,11 +3345,11 @@ mod test {
build_basic_foo_with_return(&mut builder, bar_id, true, InlineType::default());

let ssa = builder.finish();
let brillig = ssa.to_brillig(false);
let brillig = ssa.to_brillig(&BrilligOptions::default());

let (acir_functions, brillig_functions, _, _) = ssa
.generate_entry_point_index()
.into_acir(&brillig, ExpressionWidth::default())
.into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function");
Expand Down Expand Up @@ -3405,7 +3414,7 @@ mod test {
// Brillig artifacts to the ACIR gen pass.
let (acir_functions, brillig_functions, _, _) = ssa
.generate_entry_point_index()
.into_acir(&Brillig::default(), ExpressionWidth::default())
.into_acir(&Brillig::default(), &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function");
Expand Down Expand Up @@ -3475,12 +3484,12 @@ mod test {

let ssa = builder.finish();
// We need to generate Brillig artifacts for the regular Brillig function and pass them to the ACIR generation pass.
let brillig = ssa.to_brillig(false);
let brillig = ssa.to_brillig(&BrilligOptions::default());
println!("{}", ssa);

let (acir_functions, brillig_functions, _, _) = ssa
.generate_entry_point_index()
.into_acir(&brillig, ExpressionWidth::default())
.into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function");
Expand Down Expand Up @@ -3564,12 +3573,12 @@ mod test {

let ssa = builder.finish();
// We need to generate Brillig artifacts for the regular Brillig function and pass them to the ACIR generation pass.
let brillig = ssa.to_brillig(false);
let brillig = ssa.to_brillig(&BrilligOptions::default());
println!("{}", ssa);

let (acir_functions, brillig_functions, _, _) = ssa
.generate_entry_point_index()
.into_acir(&brillig, ExpressionWidth::default())
.into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions");
Expand Down Expand Up @@ -3681,10 +3690,10 @@ mod test {
}
";
let ssa = Ssa::from_str(src).unwrap();
let brillig = ssa.to_brillig(false);
let brillig = ssa.to_brillig(&BrilligOptions::default());

let (mut acir_functions, _brillig_functions, _, _) = ssa
.into_acir(&brillig, ExpressionWidth::default())
.into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 1);
Expand Down Expand Up @@ -3712,17 +3721,17 @@ mod test {
constrain v7 == Field 0
return
}

brillig(inline) fn foo f1 {
b0(v0: u32, v1: [Field]):
return
}
";
let ssa = Ssa::from_str(src).unwrap();
let brillig = ssa.to_brillig(false);
let brillig = ssa.to_brillig(&BrilligOptions::default());

let (acir_functions, _brillig_functions, _, _) = ssa
.into_acir(&brillig, ExpressionWidth::default())
.into_acir(&brillig, &BrilligOptions::default(), ExpressionWidth::default())
.expect("Should compile manually written SSA into ACIR");

assert_eq!(acir_functions.len(), 1);
Expand Down
13 changes: 9 additions & 4 deletions compiler/noirc_evaluator/src/brillig/brillig_gen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use super::{
artifact::{BrilligArtifact, BrilligParameter, GeneratedBrillig, Label},
BrilligContext,
},
Brillig, BrilligVariable, ValueId,
Brillig, BrilligOptions, BrilligVariable, ValueId,
};
use crate::{
errors::InternalError,
Expand All @@ -26,10 +26,10 @@ use crate::{
/// Converting an SSA function into Brillig bytecode.
pub(crate) fn convert_ssa_function(
func: &Function,
enable_debug_trace: bool,
options: &BrilligOptions,
globals: &HashMap<ValueId, BrilligVariable>,
) -> BrilligArtifact<FieldElement> {
let mut brillig_context = BrilligContext::new(enable_debug_trace);
let mut brillig_context = BrilligContext::new(options);

let mut function_context = FunctionContext::new(func);

Expand All @@ -56,25 +56,30 @@ pub(crate) fn gen_brillig_for(
func: &Function,
arguments: Vec<BrilligParameter>,
brillig: &Brillig,
options: &BrilligOptions,
) -> Result<GeneratedBrillig<FieldElement>, InternalError> {
// Create the entry point artifact
let globals_memory_size = brillig
.globals_memory_size
.get(&func.id())
.copied()
.expect("Should have the globals memory size specified for an entry point");

let options = BrilligOptions { enable_debug_trace: false, ..*options };

let mut entry_point = BrilligContext::new_entry_point_artifact(
arguments,
FunctionContext::return_values(func),
func.id(),
true,
globals_memory_size,
&options,
);
entry_point.name = func.name().to_string();

// Link the entry point with all dependencies
while let Some(unresolved_fn_label) = entry_point.first_unresolved_function_call() {
let artifact = &brillig.find_by_label(unresolved_fn_label.clone());
let artifact = &brillig.find_by_label(unresolved_fn_label.clone(), &options);
let artifact = match artifact {
Some(artifact) => artifact,
None => {
Expand Down
70 changes: 58 additions & 12 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 @@ -20,6 +20,7 @@ use crate::ssa::ir::{
value::{Value, ValueId},
};
use acvm::acir::brillig::{MemoryAddress, ValueOrArray};
use acvm::brillig_vm::MEMORY_ADDRESSING_BIT_SIZE;
use acvm::{acir::AcirField, FieldElement};
use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
use iter_extended::vecmap;
Expand Down Expand Up @@ -835,20 +836,65 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
.store_instruction(array_or_vector.extract_register(), rc_register);
self.brillig_context.deallocate_register(rc_register);
}
Instruction::DecrementRc { value } => {
Instruction::DecrementRc { value, original } => {
let array_or_vector = self.convert_ssa_value(*value, dfg);
let rc_register = self.brillig_context.allocate_register();
let orig_array_or_vector = self.convert_ssa_value(*original, dfg);

self.brillig_context
.load_instruction(rc_register, array_or_vector.extract_register());
self.brillig_context.codegen_usize_op_in_place(
rc_register,
BrilligBinaryOp::Sub,
1,
);
self.brillig_context
.store_instruction(array_or_vector.extract_register(), rc_register);
self.brillig_context.deallocate_register(rc_register);
let array_register = array_or_vector.extract_register();
let orig_array_register = orig_array_or_vector.extract_register();

let codegen_dec_rc = |ctx: &mut BrilligContext<FieldElement, Registers>| {
let rc_register = ctx.allocate_register();

ctx.load_instruction(rc_register, array_register);

ctx.codegen_usize_op_in_place(rc_register, BrilligBinaryOp::Sub, 1);

// Check that the refcount didn't go below 1. If we allow it to underflow
// and become 0 or usize::MAX, and then return to 1, then it will indicate
// an array as mutable when it probably shouldn't be.
if ctx.enable_debug_assertions() {
let min_addr = ReservedRegisters::usize_one();
let condition = SingleAddrVariable::new(ctx.allocate_register(), 1);
ctx.memory_op_instruction(
min_addr,
rc_register,
condition.address,
BrilligBinaryOp::LessThanEquals,
);
ctx.codegen_constrain(
condition,
Some("array ref-count went to zero".to_owned()),
);
ctx.deallocate_single_addr(condition);
}

ctx.store_instruction(array_register, rc_register);
ctx.deallocate_register(rc_register);
};

if array_register == orig_array_register {
codegen_dec_rc(self.brillig_context);
} else {
// Check if the current and original register point at the same memory.
// If they don't, it means that an array-copy procedure has created a
// new array, which includes decrementing the RC of the original,
// which means we don't have to do the decrement here.
let condition =
SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
// We will use a binary op to interpret the pointer as a number.
let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into();
let array_var = SingleAddrVariable::new(array_register, bit_size);
let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size);
self.brillig_context.binary_instruction(
array_var,
orig_array_var,
condition,
BrilligBinaryOp::Equals,
);
self.brillig_context.codegen_if(condition.address, codegen_dec_rc);
self.brillig_context.deallocate_single_addr(condition);
}
}
Instruction::EnableSideEffectsIf { .. } => {
unreachable!("enable_side_effects not supported by brillig")
Expand Down
Loading
Loading