Skip to content

Commit

Permalink
fix!: Only decrement the counter of an array if its address has not c…
Browse files Browse the repository at this point in the history 
…hanged (#7297)
  • Loading branch information
@aakoshh
aakoshh authored Feb 13, 2025
1 parent 81b86e2 commit 93d1740
Show file tree
Hide file tree
Showing 27 changed files with 418 additions and 291 deletions.
10 changes: 9 additions & 1 deletion compiler/noirc_driver/src/lib.rs
View file
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 @@ -680,7 +685,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
80 changes: 43 additions & 37 deletions compiler/noirc_evaluator/src/acir/mod.rs
View file
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 @@ -209,6 +210,11 @@ struct Context<'a> {

/// Contains state that is generated and also used across ACIR functions
shared_context: &'a mut SharedContext<FieldElement>,

brillig: &'a Brillig,

/// Options affecting Brillig code generation.
brillig_options: &'a BrilligOptions,
}

#[derive(Clone)]
Expand Down Expand Up @@ -305,17 +311,18 @@ impl Ssa {
pub(crate) fn into_acir(
self,
brillig: &Brillig,
brillig_options: &BrilligOptions,
expression_width: ExpressionWidth,
) -> Result<Artifacts, RuntimeError> {
let mut acirs = Vec::new();
// TODO: can we parallelize this?
let mut shared_context = SharedContext::default();

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)?
{
let context =
Context::new(&mut shared_context, expression_width, brillig, brillig_options);

if let Some(mut generated_acir) = context.convert_ssa_function(&self, function)? {
// 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,
// we instead store the opcode location at which a Brillig call to a std lib function occurred.
Expand Down Expand Up @@ -364,6 +371,8 @@ impl<'a> Context<'a> {
fn new(
shared_context: &'a mut SharedContext<FieldElement>,
expression_width: ExpressionWidth,
brillig: &'a Brillig,
brillig_options: &'a BrilligOptions,
) -> Context<'a> {
let mut acir_context = AcirContext::default();
acir_context.set_expression_width(expression_width);
Expand All @@ -380,14 +389,15 @@ impl<'a> Context<'a> {
max_block_id: 0,
data_bus: DataBus::default(),
shared_context,
brillig,
brillig_options,
}
}

fn convert_ssa_function(
self,
ssa: &Ssa,
function: &Function,
brillig: &Brillig,
) -> Result<Option<GeneratedAcir<FieldElement>>, RuntimeError> {
match function.runtime() {
RuntimeType::Acir(inline_type) => {
Expand All @@ -403,11 +413,11 @@ impl<'a> Context<'a> {
InlineType::Fold => {}
}
// We only want to convert entry point functions. This being `main` and those marked with `InlineType::Fold`
Ok(Some(self.convert_acir_main(function, ssa, brillig)?))
Ok(Some(self.convert_acir_main(function, ssa)?))
}
RuntimeType::Brillig(_) => {
if function.id() == ssa.main_id {
Ok(Some(self.convert_brillig_main(function, brillig)?))
Ok(Some(self.convert_brillig_main(function)?))
} else {
Ok(None)
}
Expand All @@ -419,7 +429,6 @@ impl<'a> Context<'a> {
mut self,
main_func: &Function,
ssa: &Ssa,
brillig: &Brillig,
) -> Result<GeneratedAcir<FieldElement>, RuntimeError> {
let dfg = &main_func.dfg;
let entry_block = &dfg[main_func.entry_block()];
Expand Down Expand Up @@ -447,7 +456,7 @@ impl<'a> Context<'a> {
self.data_bus = dfg.data_bus.to_owned();
let mut warnings = Vec::new();
for instruction_id in entry_block.instructions() {
warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa, brillig)?);
warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa)?);
}
let (return_vars, return_warnings) =
self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?;
Expand Down Expand Up @@ -504,7 +513,6 @@ impl<'a> Context<'a> {
fn convert_brillig_main(
mut self,
main_func: &Function,
brillig: &Brillig,
) -> Result<GeneratedAcir<FieldElement>, RuntimeError> {
let dfg = &main_func.dfg;

Expand All @@ -519,7 +527,8 @@ 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(), self.brillig, self.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 @@ -674,7 +683,6 @@ impl<'a> Context<'a> {
instruction_id: InstructionId,
dfg: &DataFlowGraph,
ssa: &Ssa,
brillig: &Brillig,
) -> Result<Vec<SsaReport>, RuntimeError> {
let instruction = &dfg[instruction_id];
self.acir_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id));
Expand Down Expand Up @@ -777,13 +785,7 @@ impl<'a> Context<'a> {
}
Instruction::Call { .. } => {
let result_ids = dfg.instruction_results(instruction_id);
warnings.extend(self.convert_ssa_call(
instruction,
dfg,
ssa,
brillig,
result_ids,
)?);
warnings.extend(self.convert_ssa_call(instruction, dfg, ssa, result_ids)?);
}
Instruction::Not(value_id) => {
let (acir_var, typ) = match self.convert_value(*value_id, dfg) {
Expand Down Expand Up @@ -849,7 +851,6 @@ impl<'a> Context<'a> {
instruction: &Instruction,
dfg: &DataFlowGraph,
ssa: &Ssa,
brillig: &Brillig,
result_ids: &[ValueId],
) -> Result<Vec<SsaReport>, RuntimeError> {
let mut warnings = Vec::new();
Expand Down Expand Up @@ -927,7 +928,12 @@ impl<'a> Context<'a> {
None,
)?
} else {
let code = gen_brillig_for(func, arguments.clone(), brillig)?;
let code = gen_brillig_for(
func,
arguments.clone(),
self.brillig,
self.brillig_options,
)?;
let generated_pointer =
self.shared_context.new_generated_pointer();
let output_values = self.acir_context.brillig_call(
Expand Down Expand Up @@ -2904,7 +2910,7 @@ mod test {

use crate::{
acir::BrilligStdlibFunc,
brillig::Brillig,
brillig::{Brillig, BrilligOptions},
ssa::{
function_builder::FunctionBuilder,
ir::{
Expand Down Expand Up @@ -3005,7 +3011,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 +3117,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 +3221,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 +3342,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 +3411,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 +3481,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 +3570,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 +3687,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 +3718,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
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
Loading

0 comments on commit 93d1740

Please sign in to comment.