From 42a6aa00428f3e9bfa2e630e0799218034c0b05d Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 14:51:15 +0000 Subject: [PATCH 01/21] working initial mvp of brillig stdlib --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 16 +++++--- .../ssa/acir_gen/acir_ir/generated_acir.rs | 40 ++++++++++++++++++- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 27 +++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index b94e02e5119..01bf25dd1f9 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -1,5 +1,5 @@ use super::big_int::BigIntContext; -use super::generated_acir::GeneratedAcir; +use super::generated_acir::{BrilligStdlibFunc, GeneratedAcir}; use crate::brillig::brillig_gen::brillig_directive; use crate::brillig::brillig_ir::artifact::GeneratedBrillig; use crate::errors::{InternalError, RuntimeError, SsaReport}; @@ -326,13 +326,15 @@ impl AcirContext { // Compute the inverse with brillig code let inverse_code = brillig_directive::directive_invert(); - let results = self.brillig( + let results = self.brillig_call( predicate, - inverse_code, + &inverse_code, vec![AcirValue::Var(var, AcirType::field())], vec![AcirType::field()], true, false, + 0, + Some(BrilligStdlibFunc::Inverse) )?; let inverted_var = Self::expect_one_var(results); @@ -711,9 +713,9 @@ impl AcirContext { } let [q_value, r_value]: [AcirValue; 2] = self - .brillig( + .brillig_call( predicate, - brillig_directive::directive_quotient(bit_size + 1), + &brillig_directive::directive_quotient(bit_size + 1), vec![ AcirValue::Var(lhs, AcirType::unsigned(bit_size)), AcirValue::Var(rhs, AcirType::unsigned(bit_size)), @@ -721,6 +723,8 @@ impl AcirContext { vec![AcirType::unsigned(max_q_bits), AcirType::unsigned(max_rhs_bits)], true, false, + 0, + Some(BrilligStdlibFunc::Quotient(bit_size + 1)) )? .try_into() .expect("quotient only returns two values"); @@ -1565,6 +1569,7 @@ impl AcirContext { attempt_execution: bool, unsafe_return_values: bool, brillig_function_index: u32, + brillig_stdlib_func: Option, ) -> Result, RuntimeError> { let brillig_inputs = try_vecmap(inputs, |i| -> Result<_, InternalError> { match i { @@ -1618,6 +1623,7 @@ impl AcirContext { brillig_inputs, brillig_outputs, brillig_function_index, + brillig_stdlib_func, ); fn range_constraint_value( diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 999ff2ddb5d..9acb2ee0334 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -62,6 +62,18 @@ pub(crate) struct GeneratedAcir { /// Name for the corresponding entry point represented by this Acir-gen output. /// Only used for debugging and benchmarking purposes pub(crate) name: String, + + /// Maps the opcode index to a Brillig std library function call. + /// As to avoid passing the ACIR gen shared context into each individual ACIR + /// we can instead keep this map and resolve the Brillig calls at the end of code generation. + pub(crate) brillig_stdlib_func_locations: BTreeMap, +} + +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] +pub(crate) enum BrilligStdlibFunc { + Inverse, + // The Brillig quotient code is different depending upon the bit size. + Quotient(u32), } impl GeneratedAcir { @@ -456,7 +468,7 @@ impl GeneratedAcir { let inverse_code = brillig_directive::directive_invert(); let inputs = vec![BrilligInputs::Single(expr)]; let outputs = vec![BrilligOutputs::Simple(inverted_witness)]; - self.brillig(Some(Expression::one()), inverse_code, inputs, outputs); + self.brillig_call(Some(Expression::one()), &inverse_code, inputs, outputs, 0, Some(BrilligStdlibFunc::Inverse)); inverted_witness } @@ -625,10 +637,18 @@ impl GeneratedAcir { inputs: Vec, outputs: Vec, brillig_function_index: u32, + stdlib_func: Option, ) { let opcode = AcirOpcode::BrilligCall { id: brillig_function_index, inputs, outputs, predicate }; self.push_opcode(opcode); + if let Some(stdlib_func) = stdlib_func { + self.brillig_stdlib_func_locations.insert(self.last_acir_opcode_location(), stdlib_func); + // Brillig stdlib functions are handwritten and do not have location or debug information attached + // to them as they are expected to be written correctly. + // return + } + for (brillig_index, call_stack) in generated_brillig.locations.iter() { self.locations.insert( OpcodeLocation::Brillig { @@ -649,6 +669,24 @@ impl GeneratedAcir { } } + // We can only resolve the Brillig stdlib after having processed the entire ACIR + pub(crate) fn resolve_brillig_stdlib_call( + &mut self, + opcode_location: OpcodeLocation, + brillig_function_index: u32, + ) { + let acir_index = match opcode_location { + OpcodeLocation::Acir(index) => index, + _ => panic!("should not have brillig index"), + }; + match &mut self.opcodes[acir_index] { + AcirOpcode::BrilligCall { id, .. } => { + *id = brillig_function_index + } + _ => panic!("expected brillig call opcode"), + } + } + pub(crate) fn last_acir_opcode_location(&self) -> OpcodeLocation { OpcodeLocation::Acir(self.opcodes.len() - 1) } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 02b381d79fc..aa77735b69c 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -5,6 +5,7 @@ use std::collections::{BTreeMap, HashSet}; use std::fmt::Debug; use self::acir_ir::acir_variable::{AcirContext, AcirType, AcirVar}; +use self::acir_ir::generated_acir::BrilligStdlibFunc; use super::function_builder::data_bus::DataBus; use super::ir::dfg::CallStack; use super::ir::function::FunctionId; @@ -22,6 +23,7 @@ use super::{ }, ssa_gen::Ssa, }; +use crate::brillig::brillig_gen::brillig_directive; use crate::brillig::brillig_ir::artifact::{BrilligParameter, GeneratedBrillig}; use crate::brillig::brillig_ir::BrilligContext; use crate::brillig::{brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, Brillig}; @@ -235,11 +237,33 @@ impl Ssa { let mut acirs = Vec::new(); // TODO: can we parallelise this? let mut shared_context = SharedContext::default(); + let mut brillig_stdlib_func_index: HashMap = HashMap::default(); + let mut opcodes_to_resolve = HashMap::default(); for function in self.functions.values() { let context = Context::new(&mut shared_context); if let Some(mut generated_acir) = context.convert_ssa_function(&self, function, brillig)? { + for (opcode_location, brillig_stdlib_func) in &generated_acir.brillig_stdlib_func_locations { + if let Some(generated_brillig_pointer) = brillig_stdlib_func_index.get(brillig_stdlib_func) { + opcodes_to_resolve.insert((function.id(), *opcode_location), *generated_brillig_pointer); + } else { + let code = match brillig_stdlib_func { + BrilligStdlibFunc::Inverse => brillig_directive::directive_invert(), + BrilligStdlibFunc::Quotient(bit_size) => brillig_directive::directive_quotient(*bit_size), + }; + let new_generated_pointer = shared_context.generated_brillig.len() as u32; + shared_context.generated_brillig.push(code); + brillig_stdlib_func_index.insert(*brillig_stdlib_func, new_generated_pointer); + opcodes_to_resolve.insert((function.id(), *opcode_location), new_generated_pointer); + } + } + // We have to do a separate loop as the generated acir cannot be borrwed as mutable after an immutable borrow. + for ((_, opcode_location), brillig_function_pointer) in opcodes_to_resolve.iter() { + generated_acir.resolve_brillig_stdlib_call(*opcode_location, *brillig_function_pointer); + } + + // TODO: do I resuse the shared context logic on this generated_acir.name = function.name().to_owned(); acirs.push(generated_acir); } @@ -376,6 +400,7 @@ impl<'a> Context<'a> { true, // We are guaranteed to have a Brillig function pointer of `0` as main itself is marked as unconstrained 0, + None, )?; self.shared_context.insert_generated_brillig(main_func.id(), arguments, 0, code); @@ -687,6 +712,7 @@ impl<'a> Context<'a> { true, false, *generated_pointer, + None, )? } else { let code = @@ -701,6 +727,7 @@ impl<'a> Context<'a> { true, false, generated_pointer, + None, )?; self.shared_context.insert_generated_brillig( *id, From 699650ef0d92b95f6c64509f09d6b9b01d10f1ee Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 16:52:20 +0000 Subject: [PATCH 02/21] cleanup Brillig stdlib context and tests --- acvm-repo/acir/src/circuit/brillig.rs | 2 +- .../ssa/acir_gen/acir_ir/generated_acir.rs | 9 + .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 260 ++++++++++++++++-- 3 files changed, 248 insertions(+), 23 deletions(-) diff --git a/acvm-repo/acir/src/circuit/brillig.rs b/acvm-repo/acir/src/circuit/brillig.rs index e75d335d52b..7f87aabf9d5 100644 --- a/acvm-repo/acir/src/circuit/brillig.rs +++ b/acvm-repo/acir/src/circuit/brillig.rs @@ -33,7 +33,7 @@ pub struct Brillig { /// This is purely a wrapper struct around a list of Brillig opcode's which represents /// a full Brillig function to be executed by the Brillig VM. /// This is stored separately on a program and accessed through a [BrilligPointer]. -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Default, Debug)] pub struct BrilligBytecode { pub bytecode: Vec, } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 9acb2ee0334..dd741fb4124 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -76,6 +76,15 @@ pub(crate) enum BrilligStdlibFunc { Quotient(u32), } +impl BrilligStdlibFunc { + pub(crate) fn get_generated_brillig(&self) -> GeneratedBrillig { + match self { + BrilligStdlibFunc::Inverse => brillig_directive::directive_invert(), + BrilligStdlibFunc::Quotient(bit_size) => brillig_directive::directive_quotient(*bit_size), + } + } +} + impl GeneratedAcir { /// Returns the current witness index. pub(crate) fn current_witness_index(&self) -> Witness { diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index aa77735b69c..f71c2f69195 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -32,6 +32,7 @@ use crate::ssa::ir::function::InlineType; pub(crate) use acir_ir::generated_acir::GeneratedAcir; use acvm::acir::circuit::brillig::BrilligBytecode; +use acvm::acir::circuit::OpcodeLocation; use acvm::acir::native_types::Witness; use acvm::acir::BlackBoxFunc; use acvm::{ @@ -57,6 +58,10 @@ struct SharedContext { /// This uses the brillig parameters in the map since using slices with different lengths /// needs to create different brillig entrypoints brillig_generated_func_pointers: BTreeMap<(FunctionId, Vec), u32>, + + brillig_stdlib_func_index: HashMap, + + brillig_stdlib_calls_to_resolve: HashMap<(FunctionId, OpcodeLocation), u32>, } impl SharedContext { @@ -86,6 +91,26 @@ impl SharedContext { fn new_generated_pointer(&self) -> u32 { self.generated_brillig.len() as u32 } + + fn generated_brillig_stdlib_pointer( + &self, + brillig_stdlib_func: &BrilligStdlibFunc, + ) -> Option<&u32> { + self.brillig_stdlib_func_index.get(brillig_stdlib_func) + } + + fn insert_generated_brillig_stdlib( + &mut self, + brillig_stdlib_func: BrilligStdlibFunc, + generated_pointer: u32, + func_id: FunctionId, + opcode_location: OpcodeLocation, + code: GeneratedBrillig, + ) { + self.brillig_stdlib_func_index.insert(brillig_stdlib_func, generated_pointer); + self.brillig_stdlib_calls_to_resolve.insert((func_id, opcode_location), generated_pointer); + self.generated_brillig.push(code); + } } /// Context struct for the acir generation pass. @@ -237,33 +262,30 @@ impl Ssa { let mut acirs = Vec::new(); // TODO: can we parallelise this? let mut shared_context = SharedContext::default(); - let mut brillig_stdlib_func_index: HashMap = HashMap::default(); - let mut opcodes_to_resolve = HashMap::default(); for function in self.functions.values() { let context = Context::new(&mut shared_context); if let Some(mut generated_acir) = context.convert_ssa_function(&self, function, brillig)? { + // 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 do 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. + // We then defer resolving the function IDs of those Brillig functions to when we have generated Brillig + // for all normal Brillig calls. for (opcode_location, brillig_stdlib_func) in &generated_acir.brillig_stdlib_func_locations { - if let Some(generated_brillig_pointer) = brillig_stdlib_func_index.get(brillig_stdlib_func) { - opcodes_to_resolve.insert((function.id(), *opcode_location), *generated_brillig_pointer); + if let Some(generated_pointer) = shared_context.generated_brillig_stdlib_pointer(brillig_stdlib_func) { + shared_context.brillig_stdlib_calls_to_resolve.insert((function.id(), *opcode_location), *generated_pointer); } else { - let code = match brillig_stdlib_func { - BrilligStdlibFunc::Inverse => brillig_directive::directive_invert(), - BrilligStdlibFunc::Quotient(bit_size) => brillig_directive::directive_quotient(*bit_size), - }; - let new_generated_pointer = shared_context.generated_brillig.len() as u32; - shared_context.generated_brillig.push(code); - brillig_stdlib_func_index.insert(*brillig_stdlib_func, new_generated_pointer); - opcodes_to_resolve.insert((function.id(), *opcode_location), new_generated_pointer); + let code = brillig_stdlib_func.get_generated_brillig(); + let generated_pointer = shared_context.new_generated_pointer(); + shared_context.insert_generated_brillig_stdlib(*brillig_stdlib_func, generated_pointer, function.id(), *opcode_location, code); } } - // We have to do a separate loop as the generated acir cannot be borrwed as mutable after an immutable borrow. - for ((_, opcode_location), brillig_function_pointer) in opcodes_to_resolve.iter() { + // We have to do a separate loop as the generated ACIR cannot be borrowed as mutable after an immutable borrow + for ((_, opcode_location), brillig_function_pointer) in shared_context.brillig_stdlib_calls_to_resolve.iter() { generated_acir.resolve_brillig_stdlib_call(*opcode_location, *brillig_function_pointer); } - // TODO: do I resuse the shared context logic on this generated_acir.name = function.name().to_owned(); acirs.push(generated_acir); } @@ -2546,20 +2568,19 @@ fn can_omit_element_sizes_array(array_typ: &Type) -> bool { #[cfg(test)] mod test { use acvm::{ - acir::{circuit::Opcode, native_types::Witness}, + acir::{circuit::{Opcode, OpcodeLocation}, native_types::Witness}, FieldElement, }; use crate::{ brillig::Brillig, ssa::{ - function_builder::FunctionBuilder, - ir::{ + acir_gen::acir_ir::generated_acir::BrilligStdlibFunc, function_builder::FunctionBuilder, ir::{ function::{FunctionId, InlineType}, instruction::BinaryOp, map::Id, types::Type, - }, + } }, }; @@ -2875,11 +2896,13 @@ mod test { fn multiple_brillig_calls_one_bytecode() { // acir(inline) fn main f0 { // b0(v0: Field, v1: Field): - // v3 = call f1(v0, v1) // v4 = call f1(v0, v1) // v5 = call f1(v0, v1) // v6 = call f1(v0, v1) - // return + // v7 = call f2(v0, v1) + // v8 = call f1(v0, v1) + // v9 = call f2(v0, v1) + // return // } // brillig fn foo f1 { // b0(v0: Field, v1: Field): @@ -2928,7 +2951,7 @@ mod test { assert_eq!( brillig_functions.len(), 2, - "Should only have generated a single Brillig function" + "Should only have generated two Brillig functions" ); let main_acir = &acir_functions[0]; @@ -2946,4 +2969,197 @@ mod test { } } } + + // Test that given multiple primitive operations that are represented by Brillig directives (e.g. invert/quotient), + // we will only generate one bytecode and the appropriate Brillig call opcodes are generated. + #[test] + fn multiple_brillig_stdlib_calls() { + // acir(inline) fn main f0 { + // b0(v0: u32, v1: u32, v2: u32): + // v3 = div v0, v1 + // constrain v3 == v2 + // v4 = div v0, v1 + // constrain v4 == u32 1 + // return + // } + let foo_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), foo_id); + let main_v0 = builder.add_parameter(Type::unsigned(32)); + let main_v1 = builder.add_parameter(Type::unsigned(32)); + let main_v2 = builder.add_parameter(Type::unsigned(32)); + + // Call a primitive operation that uses Brillig + let v0_div_v1 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + builder.insert_constrain(v0_div_v1, main_v2, None); + + // Call the same primitive operation again + let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + let one = builder.numeric_constant(1u128, Type::unsigned(32)); + builder.insert_constrain(v2_div_v2, one, None); + + builder.terminate_with_return(vec![]); + + let ssa = builder.finish(); + println!("{}", ssa); + + // The Brillig bytecode we insert for the stdlib is hardcoded so we do not need to provide any + // Brillig artifacts to the ACIR gen pass. + let (acir_functions, brillig_functions) = ssa + .into_acir(&Brillig::default(), noirc_frontend::Distinctness::Distinct) + .expect("Should compile manually written SSA into ACIR"); + + assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); + assert_eq!( + brillig_functions.len(), + 2, + "Should only have generated two Brillig functions" + ); + + let main_acir = &acir_functions[0]; + let main_opcodes = main_acir.opcodes(); + + let mut num_brillig_stdlib_calls = 0; + let brillig_stdlib_function_locations = &acir_functions[0].brillig_stdlib_func_locations; + for (i, (opcode_location, brillig_stdlib_func)) in brillig_stdlib_function_locations.iter().enumerate() { + // We can take just modulo 2 to determine the expected ID as we only code generated two Brillig stdlib function + let stdlib_func_index = (i % 2) as u32; + if stdlib_func_index == 0 { + assert!(matches!(brillig_stdlib_func, BrilligStdlibFunc::Inverse)); + } else { + assert!(matches!(brillig_stdlib_func, BrilligStdlibFunc::Quotient(_))); + } + + match opcode_location { + OpcodeLocation::Acir(acir_index) => { + match main_opcodes[*acir_index] { + Opcode::BrilligCall { id, .. } => { + assert_eq!(id, stdlib_func_index, "Expected {stdlib_func_index} but got {id}"); + num_brillig_stdlib_calls += 1; + } + _ => panic!("Expected BrilligCall opcode"), + } + } + _ => panic!("Expected OpcodeLocation::Acir"), + } + } + + assert_eq!(num_brillig_stdlib_calls, 4, "Should have four BrilligCall opcodes to stdlib functions"); + } + + // Test that given both hardcoded Brillig directives and calls to normal Brillig functions, + // we generate a single bytecode for the directives and a single bytecode for the normal Brillig calls. + #[test] + fn brillig_stdlib_calls_with_regular_brillig_call() { + // acir(inline) fn main f0 { + // b0(v0: u32, v1: u32, v2: u32): + // v4 = div v0, v1 + // constrain v4 == v2 + // v5 = call f1(v0, v1) + // v6 = call f1(v0, v1) + // v7 = div v0, v1 + // constrain v7 == u32 1 + // return + // } + // brillig fn foo f1 { + // b0(v0: Field, v1: Field): + // v2 = eq v0, v1 + // constrain v2 == u1 0 + // return v0 + // } + let foo_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), foo_id); + let main_v0 = builder.add_parameter(Type::unsigned(32)); + let main_v1 = builder.add_parameter(Type::unsigned(32)); + let main_v2 = builder.add_parameter(Type::unsigned(32)); + + let foo_id = Id::test_new(1); + let foo = builder.import_function(foo_id); + + // Call a primitive operation that uses Brillig + let v0_div_v1 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + builder.insert_constrain(v0_div_v1, main_v2, None); + + // Insert multiple calls to the same Brillig function + builder.insert_call(foo, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); + builder.insert_call(foo, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); + + // Call the same primitive operation again + let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + let one = builder.numeric_constant(1u128, Type::unsigned(32)); + builder.insert_constrain(v2_div_v2, one, None); + + builder.terminate_with_return(vec![]); + + build_basic_foo_with_return(&mut builder, foo_id, true); + + 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); + println!("{}", ssa); + + let (acir_functions, brillig_functions) = ssa + .into_acir(&brillig, noirc_frontend::Distinctness::Distinct) + .expect("Should compile manually written SSA into ACIR"); + + assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); + assert_eq!( + brillig_functions.len(), + 3, + "Should only have generated three Brillig functions" + ); + + let main_acir = &acir_functions[0]; + let main_opcodes = main_acir.opcodes(); + + let mut num_brillig_stdlib_calls = 0; + let brillig_stdlib_function_locations = &acir_functions[0].brillig_stdlib_func_locations; + for (i, (opcode_location, brillig_stdlib_func)) in brillig_stdlib_function_locations.iter().enumerate() { + // We can take just modulo 2 to determine the expected ID as we only code generated two Brillig stdlib function + let stdlib_func_index = (i % 2) as u32; + if stdlib_func_index == 0 { + assert!(matches!(brillig_stdlib_func, BrilligStdlibFunc::Inverse)); + } else { + assert!(matches!(brillig_stdlib_func, BrilligStdlibFunc::Quotient(_))); + } + + match opcode_location { + OpcodeLocation::Acir(acir_index) => { + match main_opcodes[*acir_index] { + Opcode::BrilligCall { id, .. } => { + // Brillig stdlib function calls are only resolved at the end of ACIR generation so their + // IDs are expected to always reference Brillig bytecode at the end of the Brillig functions list. + // We have one normal Brillig call so we add one here to the std lib function's index within the std lib. + let expected_id = stdlib_func_index + 1; + assert_eq!(id, expected_id, "Expected {expected_id} but got {id}"); + num_brillig_stdlib_calls += 1; + } + _ => panic!("Expected BrilligCall opcode"), + } + } + _ => panic!("Expected OpcodeLocation::Acir"), + } + } + + assert_eq!(num_brillig_stdlib_calls, 4, "Should have four BrilligCall opcodes to stdlib functions"); + + // Check the normal Brillig call + let mut num_normal_brillig_calls = 0; + for (i, opcode) in main_opcodes.iter().enumerate() { + match opcode { + Opcode::BrilligCall { id, .. } => { + if brillig_stdlib_function_locations.get(&OpcodeLocation::Acir(i)).is_some() { + // We should have already checked Brillig stdlib functions and only want to check normal Brillig calls here + continue + } + // We only generate one normal Brillig call so we should expect a function ID of `0` + let expected_id = 0u32; + assert_eq!(*id, expected_id, "Expected an id of {expected_id} but got {id}"); + num_normal_brillig_calls += 1; + } + _ => {}, + } + } + + assert_eq!(num_normal_brillig_calls, 2, "Should have two BrilligCall opcodes to normal Brillig functions"); + } } From a663f542331d781beb2fed42b7826b86876271f7 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 16:54:50 +0000 Subject: [PATCH 03/21] comment cleanup and fmt --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 4 +- .../ssa/acir_gen/acir_ir/generated_acir.rs | 25 ++-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 113 +++++++++++------- 3 files changed, 86 insertions(+), 56 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 01bf25dd1f9..e071e201adf 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -334,7 +334,7 @@ impl AcirContext { true, false, 0, - Some(BrilligStdlibFunc::Inverse) + Some(BrilligStdlibFunc::Inverse), )?; let inverted_var = Self::expect_one_var(results); @@ -724,7 +724,7 @@ impl AcirContext { true, false, 0, - Some(BrilligStdlibFunc::Quotient(bit_size + 1)) + Some(BrilligStdlibFunc::Quotient(bit_size + 1)), )? .try_into() .expect("quotient only returns two values"); diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index dd741fb4124..7508347e929 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -64,7 +64,7 @@ pub(crate) struct GeneratedAcir { pub(crate) name: String, /// Maps the opcode index to a Brillig std library function call. - /// As to avoid passing the ACIR gen shared context into each individual ACIR + /// As to avoid passing the ACIR gen shared context into each individual ACIR /// we can instead keep this map and resolve the Brillig calls at the end of code generation. pub(crate) brillig_stdlib_func_locations: BTreeMap, } @@ -80,7 +80,9 @@ impl BrilligStdlibFunc { pub(crate) fn get_generated_brillig(&self) -> GeneratedBrillig { match self { BrilligStdlibFunc::Inverse => brillig_directive::directive_invert(), - BrilligStdlibFunc::Quotient(bit_size) => brillig_directive::directive_quotient(*bit_size), + BrilligStdlibFunc::Quotient(bit_size) => { + brillig_directive::directive_quotient(*bit_size) + } } } } @@ -477,7 +479,14 @@ impl GeneratedAcir { let inverse_code = brillig_directive::directive_invert(); let inputs = vec![BrilligInputs::Single(expr)]; let outputs = vec![BrilligOutputs::Simple(inverted_witness)]; - self.brillig_call(Some(Expression::one()), &inverse_code, inputs, outputs, 0, Some(BrilligStdlibFunc::Inverse)); + self.brillig_call( + Some(Expression::one()), + &inverse_code, + inputs, + outputs, + 0, + Some(BrilligStdlibFunc::Inverse), + ); inverted_witness } @@ -652,10 +661,8 @@ impl GeneratedAcir { AcirOpcode::BrilligCall { id: brillig_function_index, inputs, outputs, predicate }; self.push_opcode(opcode); if let Some(stdlib_func) = stdlib_func { - self.brillig_stdlib_func_locations.insert(self.last_acir_opcode_location(), stdlib_func); - // Brillig stdlib functions are handwritten and do not have location or debug information attached - // to them as they are expected to be written correctly. - // return + self.brillig_stdlib_func_locations + .insert(self.last_acir_opcode_location(), stdlib_func); } for (brillig_index, call_stack) in generated_brillig.locations.iter() { @@ -689,9 +696,7 @@ impl GeneratedAcir { _ => panic!("should not have brillig index"), }; match &mut self.opcodes[acir_index] { - AcirOpcode::BrilligCall { id, .. } => { - *id = brillig_function_index - } + AcirOpcode::BrilligCall { id, .. } => *id = brillig_function_index, _ => panic!("expected brillig call opcode"), } } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index f71c2f69195..ced13978d13 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -23,7 +23,6 @@ use super::{ }, ssa_gen::Ssa, }; -use crate::brillig::brillig_gen::brillig_directive; use crate::brillig::brillig_ir::artifact::{BrilligParameter, GeneratedBrillig}; use crate::brillig::brillig_ir::BrilligContext; use crate::brillig::{brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, Brillig}; @@ -268,22 +267,37 @@ impl Ssa { context.convert_ssa_function(&self, function, brillig)? { // 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 do want a reference to the `SharedContext` on the generated ACIR itself, + // As we do 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. - // We then defer resolving the function IDs of those Brillig functions to when we have generated Brillig + // We then defer resolving the function IDs of those Brillig functions to when we have generated Brillig // for all normal Brillig calls. - for (opcode_location, brillig_stdlib_func) in &generated_acir.brillig_stdlib_func_locations { - if let Some(generated_pointer) = shared_context.generated_brillig_stdlib_pointer(brillig_stdlib_func) { - shared_context.brillig_stdlib_calls_to_resolve.insert((function.id(), *opcode_location), *generated_pointer); + for (opcode_location, brillig_stdlib_func) in + &generated_acir.brillig_stdlib_func_locations + { + if let Some(generated_pointer) = + shared_context.generated_brillig_stdlib_pointer(brillig_stdlib_func) + { + shared_context + .brillig_stdlib_calls_to_resolve + .insert((function.id(), *opcode_location), *generated_pointer); } else { let code = brillig_stdlib_func.get_generated_brillig(); let generated_pointer = shared_context.new_generated_pointer(); - shared_context.insert_generated_brillig_stdlib(*brillig_stdlib_func, generated_pointer, function.id(), *opcode_location, code); + shared_context.insert_generated_brillig_stdlib( + *brillig_stdlib_func, + generated_pointer, + function.id(), + *opcode_location, + code, + ); } } // We have to do a separate loop as the generated ACIR cannot be borrowed as mutable after an immutable borrow - for ((_, opcode_location), brillig_function_pointer) in shared_context.brillig_stdlib_calls_to_resolve.iter() { - generated_acir.resolve_brillig_stdlib_call(*opcode_location, *brillig_function_pointer); + for ((_, opcode_location), brillig_function_pointer) in + shared_context.brillig_stdlib_calls_to_resolve.iter() + { + generated_acir + .resolve_brillig_stdlib_call(*opcode_location, *brillig_function_pointer); } generated_acir.name = function.name().to_owned(); @@ -2568,19 +2582,24 @@ fn can_omit_element_sizes_array(array_typ: &Type) -> bool { #[cfg(test)] mod test { use acvm::{ - acir::{circuit::{Opcode, OpcodeLocation}, native_types::Witness}, + acir::{ + circuit::{Opcode, OpcodeLocation}, + native_types::Witness, + }, FieldElement, }; use crate::{ brillig::Brillig, ssa::{ - acir_gen::acir_ir::generated_acir::BrilligStdlibFunc, function_builder::FunctionBuilder, ir::{ + acir_gen::acir_ir::generated_acir::BrilligStdlibFunc, + function_builder::FunctionBuilder, + ir::{ function::{FunctionId, InlineType}, instruction::BinaryOp, map::Id, types::Type, - } + }, }, }; @@ -2902,7 +2921,7 @@ mod test { // v7 = call f2(v0, v1) // v8 = call f1(v0, v1) // v9 = call f2(v0, v1) - // return + // return // } // brillig fn foo f1 { // b0(v0: Field, v1: Field): @@ -2948,11 +2967,7 @@ mod test { .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); - assert_eq!( - brillig_functions.len(), - 2, - "Should only have generated two Brillig functions" - ); + assert_eq!(brillig_functions.len(), 2, "Should only have generated two Brillig functions"); let main_acir = &acir_functions[0]; let main_opcodes = main_acir.opcodes(); @@ -2980,7 +2995,7 @@ mod test { // constrain v3 == v2 // v4 = div v0, v1 // constrain v4 == u32 1 - // return + // return // } let foo_id = Id::test_new(0); let mut builder = FunctionBuilder::new("main".into(), foo_id); @@ -2990,12 +3005,12 @@ mod test { // Call a primitive operation that uses Brillig let v0_div_v1 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); - builder.insert_constrain(v0_div_v1, main_v2, None); + builder.insert_constrain(v0_div_v1, main_v2, None); // Call the same primitive operation again let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - builder.insert_constrain(v2_div_v2, one, None); + builder.insert_constrain(v2_div_v2, one, None); builder.terminate_with_return(vec![]); @@ -3009,18 +3024,16 @@ mod test { .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); - assert_eq!( - brillig_functions.len(), - 2, - "Should only have generated two Brillig functions" - ); + assert_eq!(brillig_functions.len(), 2, "Should only have generated two Brillig functions"); let main_acir = &acir_functions[0]; let main_opcodes = main_acir.opcodes(); let mut num_brillig_stdlib_calls = 0; let brillig_stdlib_function_locations = &acir_functions[0].brillig_stdlib_func_locations; - for (i, (opcode_location, brillig_stdlib_func)) in brillig_stdlib_function_locations.iter().enumerate() { + for (i, (opcode_location, brillig_stdlib_func)) in + brillig_stdlib_function_locations.iter().enumerate() + { // We can take just modulo 2 to determine the expected ID as we only code generated two Brillig stdlib function let stdlib_func_index = (i % 2) as u32; if stdlib_func_index == 0 { @@ -3030,20 +3043,24 @@ mod test { } match opcode_location { - OpcodeLocation::Acir(acir_index) => { - match main_opcodes[*acir_index] { - Opcode::BrilligCall { id, .. } => { - assert_eq!(id, stdlib_func_index, "Expected {stdlib_func_index} but got {id}"); - num_brillig_stdlib_calls += 1; - } - _ => panic!("Expected BrilligCall opcode"), + OpcodeLocation::Acir(acir_index) => match main_opcodes[*acir_index] { + Opcode::BrilligCall { id, .. } => { + assert_eq!( + id, stdlib_func_index, + "Expected {stdlib_func_index} but got {id}" + ); + num_brillig_stdlib_calls += 1; } - } + _ => panic!("Expected BrilligCall opcode"), + }, _ => panic!("Expected OpcodeLocation::Acir"), } } - assert_eq!(num_brillig_stdlib_calls, 4, "Should have four BrilligCall opcodes to stdlib functions"); + assert_eq!( + num_brillig_stdlib_calls, 4, + "Should have four BrilligCall opcodes to stdlib functions" + ); } // Test that given both hardcoded Brillig directives and calls to normal Brillig functions, @@ -3058,7 +3075,7 @@ mod test { // v6 = call f1(v0, v1) // v7 = div v0, v1 // constrain v7 == u32 1 - // return + // return // } // brillig fn foo f1 { // b0(v0: Field, v1: Field): @@ -3077,7 +3094,7 @@ mod test { // Call a primitive operation that uses Brillig let v0_div_v1 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); - builder.insert_constrain(v0_div_v1, main_v2, None); + builder.insert_constrain(v0_div_v1, main_v2, None); // Insert multiple calls to the same Brillig function builder.insert_call(foo, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); @@ -3086,7 +3103,7 @@ mod test { // Call the same primitive operation again let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - builder.insert_constrain(v2_div_v2, one, None); + builder.insert_constrain(v2_div_v2, one, None); builder.terminate_with_return(vec![]); @@ -3113,7 +3130,9 @@ mod test { let mut num_brillig_stdlib_calls = 0; let brillig_stdlib_function_locations = &acir_functions[0].brillig_stdlib_func_locations; - for (i, (opcode_location, brillig_stdlib_func)) in brillig_stdlib_function_locations.iter().enumerate() { + for (i, (opcode_location, brillig_stdlib_func)) in + brillig_stdlib_function_locations.iter().enumerate() + { // We can take just modulo 2 to determine the expected ID as we only code generated two Brillig stdlib function let stdlib_func_index = (i % 2) as u32; if stdlib_func_index == 0 { @@ -3140,7 +3159,10 @@ mod test { } } - assert_eq!(num_brillig_stdlib_calls, 4, "Should have four BrilligCall opcodes to stdlib functions"); + assert_eq!( + num_brillig_stdlib_calls, 4, + "Should have four BrilligCall opcodes to stdlib functions" + ); // Check the normal Brillig call let mut num_normal_brillig_calls = 0; @@ -3149,17 +3171,20 @@ mod test { Opcode::BrilligCall { id, .. } => { if brillig_stdlib_function_locations.get(&OpcodeLocation::Acir(i)).is_some() { // We should have already checked Brillig stdlib functions and only want to check normal Brillig calls here - continue + continue; } // We only generate one normal Brillig call so we should expect a function ID of `0` let expected_id = 0u32; assert_eq!(*id, expected_id, "Expected an id of {expected_id} but got {id}"); num_normal_brillig_calls += 1; } - _ => {}, + _ => {} } } - assert_eq!(num_normal_brillig_calls, 2, "Should have two BrilligCall opcodes to normal Brillig functions"); + assert_eq!( + num_normal_brillig_calls, 2, + "Should have two BrilligCall opcodes to normal Brillig functions" + ); } } From 04e3aa8c45e5854943fa23369d1ef35b2af1df9c Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 17:39:19 +0000 Subject: [PATCH 04/21] one more unit test and a fix --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 173 +++++++++++++----- 1 file changed, 128 insertions(+), 45 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ced13978d13..a8d175ad567 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -293,11 +293,15 @@ impl Ssa { } } // We have to do a separate loop as the generated ACIR cannot be borrowed as mutable after an immutable borrow - for ((_, opcode_location), brillig_function_pointer) in + for ((func_id, opcode_location), brillig_function_pointer) in shared_context.brillig_stdlib_calls_to_resolve.iter() { - generated_acir - .resolve_brillig_stdlib_call(*opcode_location, *brillig_function_pointer); + if *func_id == function.id() { + generated_acir.resolve_brillig_stdlib_call( + *opcode_location, + *brillig_function_pointer, + ); + } } generated_acir.name = function.name().to_owned(); @@ -2581,6 +2585,8 @@ fn can_omit_element_sizes_array(array_typ: &Type) -> bool { #[cfg(test)] mod test { + use std::collections::BTreeMap; + use acvm::{ acir::{ circuit::{Opcode, OpcodeLocation}, @@ -2848,7 +2854,7 @@ mod test { let ssa = builder.finish(); - let (acir_functions, _) = ssa + let (acir_functions, brillig_functions) = ssa .into_acir(&Brillig::default(), noirc_frontend::Distinctness::Distinct) .expect("Should compile manually written SSA into ACIR"); @@ -3028,38 +3034,12 @@ mod test { let main_acir = &acir_functions[0]; let main_opcodes = main_acir.opcodes(); - - let mut num_brillig_stdlib_calls = 0; - let brillig_stdlib_function_locations = &acir_functions[0].brillig_stdlib_func_locations; - for (i, (opcode_location, brillig_stdlib_func)) in - brillig_stdlib_function_locations.iter().enumerate() - { - // We can take just modulo 2 to determine the expected ID as we only code generated two Brillig stdlib function - let stdlib_func_index = (i % 2) as u32; - if stdlib_func_index == 0 { - assert!(matches!(brillig_stdlib_func, BrilligStdlibFunc::Inverse)); - } else { - assert!(matches!(brillig_stdlib_func, BrilligStdlibFunc::Quotient(_))); - } - - match opcode_location { - OpcodeLocation::Acir(acir_index) => match main_opcodes[*acir_index] { - Opcode::BrilligCall { id, .. } => { - assert_eq!( - id, stdlib_func_index, - "Expected {stdlib_func_index} but got {id}" - ); - num_brillig_stdlib_calls += 1; - } - _ => panic!("Expected BrilligCall opcode"), - }, - _ => panic!("Expected OpcodeLocation::Acir"), - } - } - - assert_eq!( - num_brillig_stdlib_calls, 4, - "Should have four BrilligCall opcodes to stdlib functions" + check_brillig_calls( + &acir_functions[0].brillig_stdlib_func_locations, + main_opcodes, + 0, + 4, + 0, ); } @@ -3127,9 +3107,111 @@ mod test { let main_acir = &acir_functions[0]; let main_opcodes = main_acir.opcodes(); + check_brillig_calls( + &acir_functions[0].brillig_stdlib_func_locations, + main_opcodes, + 1, + 4, + 2, + ); + } + + // Test that given both normal Brillig calls, Brillig stdlib calls, and non-inlined ACIR calls, that we accurately generate ACIR. + #[test] + fn brillig_stdlib_calls_with_multiple_acir_calls() { + // acir(inline) fn main f0 { + // b0(v0: u32, v1: u32, v2: u32): + // v4 = div v0, v1 + // constrain v4 == v2 + // v5 = call f1(v0, v1) + // v6 = call f2(v0, v1) + // v7 = div v0, v1 + // constrain v7 == u32 1 + // return + // } + // brillig fn foo f1 { + // b0(v0: Field, v1: Field): + // v2 = eq v0, v1 + // constrain v2 == u1 0 + // return v0 + // } + // acir(fold) fn foo f2 { + // b0(v0: Field, v1: Field): + // v2 = eq v0, v1 + // constrain v2 == u1 0 + // return v0 + // } + // } + let foo_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), foo_id); + let main_v0 = builder.add_parameter(Type::unsigned(32)); + let main_v1 = builder.add_parameter(Type::unsigned(32)); + let main_v2 = builder.add_parameter(Type::unsigned(32)); + + let foo_id = Id::test_new(1); + let foo = builder.import_function(foo_id); + let bar_id = Id::test_new(2); + let bar = builder.import_function(bar_id); + + // Call a primitive operation that uses Brillig + let v0_div_v1 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + builder.insert_constrain(v0_div_v1, main_v2, None); + + // Insert multiple calls to the same Brillig function + builder.insert_call(foo, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); + builder.insert_call(foo, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); + builder.insert_call(bar, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); + + // Call the same primitive operation again + let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + let one = builder.numeric_constant(1u128, Type::unsigned(32)); + builder.insert_constrain(v2_div_v2, one, None); + + builder.terminate_with_return(vec![]); + + build_basic_foo_with_return(&mut builder, foo_id, true); + build_basic_foo_with_return(&mut builder, bar_id, false); + + 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); + println!("{}", ssa); + + let (acir_functions, brillig_functions) = ssa + .into_acir(&brillig, noirc_frontend::Distinctness::Distinct) + .expect("Should compile manually written SSA into ACIR"); + + assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions"); + assert_eq!( + brillig_functions.len(), + 3, + "Should only have generated three Brillig functions" + ); + + let main_acir = &acir_functions[0]; + let main_opcodes = main_acir.opcodes(); + check_brillig_calls( + &acir_functions[0].brillig_stdlib_func_locations, + main_opcodes, + 1, + 4, + 2, + ); + + let foo_acir = &acir_functions[1]; + let foo_opcodes = foo_acir.opcodes(); + check_brillig_calls(&acir_functions[1].brillig_stdlib_func_locations, foo_opcodes, 1, 1, 0); + } + fn check_brillig_calls( + brillig_stdlib_function_locations: &BTreeMap, + opcodes: &[Opcode], + num_normal_brillig_functions: u32, + expected_num_stdlib_calls: u32, + expected_num_normal_calls: u32, + ) { + // First we check calls to the Brillig stdlib let mut num_brillig_stdlib_calls = 0; - let brillig_stdlib_function_locations = &acir_functions[0].brillig_stdlib_func_locations; for (i, (opcode_location, brillig_stdlib_func)) in brillig_stdlib_function_locations.iter().enumerate() { @@ -3143,12 +3225,12 @@ mod test { match opcode_location { OpcodeLocation::Acir(acir_index) => { - match main_opcodes[*acir_index] { + match opcodes[*acir_index] { Opcode::BrilligCall { id, .. } => { // Brillig stdlib function calls are only resolved at the end of ACIR generation so their // IDs are expected to always reference Brillig bytecode at the end of the Brillig functions list. // We have one normal Brillig call so we add one here to the std lib function's index within the std lib. - let expected_id = stdlib_func_index + 1; + let expected_id = stdlib_func_index + num_normal_brillig_functions; assert_eq!(id, expected_id, "Expected {expected_id} but got {id}"); num_brillig_stdlib_calls += 1; } @@ -3160,13 +3242,14 @@ mod test { } assert_eq!( - num_brillig_stdlib_calls, 4, - "Should have four BrilligCall opcodes to stdlib functions" + num_brillig_stdlib_calls, expected_num_stdlib_calls, + "Should have {expected_num_stdlib_calls} BrilligCall opcodes to stdlib functions but got {num_brillig_stdlib_calls}" ); - // Check the normal Brillig call + // Check the normal Brillig calls + // This check right now expects to only call one Brillig function. let mut num_normal_brillig_calls = 0; - for (i, opcode) in main_opcodes.iter().enumerate() { + for (i, opcode) in opcodes.iter().enumerate() { match opcode { Opcode::BrilligCall { id, .. } => { if brillig_stdlib_function_locations.get(&OpcodeLocation::Acir(i)).is_some() { @@ -3183,8 +3266,8 @@ mod test { } assert_eq!( - num_normal_brillig_calls, 2, - "Should have two BrilligCall opcodes to normal Brillig functions" + num_normal_brillig_calls, expected_num_normal_calls, + "Should have {expected_num_normal_calls} BrilligCall opcodes to normal Brillig functions but got {num_normal_brillig_calls}" ); } } From 5ddef4e763ab3a4af58150a0ecc3fda6c3a7f583 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 18:07:35 +0000 Subject: [PATCH 05/21] new method as to not go through every call to resolve every time --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a8d175ad567..413c803f9d7 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -60,7 +60,7 @@ struct SharedContext { brillig_stdlib_func_index: HashMap, - brillig_stdlib_calls_to_resolve: HashMap<(FunctionId, OpcodeLocation), u32>, + brillig_stdlib_calls_to_resolve: HashMap>, } impl SharedContext { @@ -107,7 +107,12 @@ impl SharedContext { code: GeneratedBrillig, ) { self.brillig_stdlib_func_index.insert(brillig_stdlib_func, generated_pointer); - self.brillig_stdlib_calls_to_resolve.insert((func_id, opcode_location), generated_pointer); + if let Some(calls_to_resolve) = self.brillig_stdlib_calls_to_resolve.get_mut(&func_id) { + calls_to_resolve.push((opcode_location, generated_pointer)); + } else { + self.brillig_stdlib_calls_to_resolve + .insert(func_id, vec![(opcode_location, generated_pointer)]); + } self.generated_brillig.push(code); } } @@ -274,12 +279,13 @@ impl Ssa { for (opcode_location, brillig_stdlib_func) in &generated_acir.brillig_stdlib_func_locations { + let mut call_to_existing_bytecode = None; if let Some(generated_pointer) = shared_context.generated_brillig_stdlib_pointer(brillig_stdlib_func) { - shared_context - .brillig_stdlib_calls_to_resolve - .insert((function.id(), *opcode_location), *generated_pointer); + // We cannot insert in the Brillig calls to resolve map here as we are doing an immutable borrow + // to the shared context to fetch the pre-existing generated pointer. + call_to_existing_bytecode = Some((*opcode_location, *generated_pointer)); } else { let code = brillig_stdlib_func.get_generated_brillig(); let generated_pointer = shared_context.new_generated_pointer(); @@ -291,12 +297,22 @@ impl Ssa { code, ); } + if let Some(new_call_to_resolve) = call_to_existing_bytecode { + if let Some(calls_to_resolve) = + shared_context.brillig_stdlib_calls_to_resolve.get_mut(&function.id()) + { + calls_to_resolve.push(new_call_to_resolve); + } + } } - // We have to do a separate loop as the generated ACIR cannot be borrowed as mutable after an immutable borrow - for ((func_id, opcode_location), brillig_function_pointer) in - shared_context.brillig_stdlib_calls_to_resolve.iter() + + // Fetch the Brillig stdlib calls to resolve for this function + if let Some(calls_to_resolve) = + shared_context.brillig_stdlib_calls_to_resolve.get(&function.id()) { - if *func_id == function.id() { + // Resolve the Brillig stdlib calls + // We have to do a separate loop as the generated ACIR cannot be borrowed as mutable after an immutable borrow + for (opcode_location, brillig_function_pointer) in calls_to_resolve { generated_acir.resolve_brillig_stdlib_call( *opcode_location, *brillig_function_pointer, From 1a2948acc4adaac2da823b6d9298cbfaa7ab683e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 18:21:49 +0000 Subject: [PATCH 06/21] fix add_call_to_resolve logic --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 413c803f9d7..aa740d4bf7f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -107,13 +107,16 @@ impl SharedContext { code: GeneratedBrillig, ) { self.brillig_stdlib_func_index.insert(brillig_stdlib_func, generated_pointer); + self.add_call_to_resolve(func_id, (opcode_location, generated_pointer)); + self.generated_brillig.push(code); + } + + fn add_call_to_resolve(&mut self, func_id: FunctionId, call_to_resolve: (OpcodeLocation, u32)) { if let Some(calls_to_resolve) = self.brillig_stdlib_calls_to_resolve.get_mut(&func_id) { - calls_to_resolve.push((opcode_location, generated_pointer)); + calls_to_resolve.push(call_to_resolve); } else { - self.brillig_stdlib_calls_to_resolve - .insert(func_id, vec![(opcode_location, generated_pointer)]); + self.brillig_stdlib_calls_to_resolve.insert(func_id, vec![call_to_resolve]); } - self.generated_brillig.push(code); } } @@ -298,11 +301,7 @@ impl Ssa { ); } if let Some(new_call_to_resolve) = call_to_existing_bytecode { - if let Some(calls_to_resolve) = - shared_context.brillig_stdlib_calls_to_resolve.get_mut(&function.id()) - { - calls_to_resolve.push(new_call_to_resolve); - } + shared_context.add_call_to_resolve(function.id(), new_call_to_resolve); } } @@ -2870,7 +2869,7 @@ mod test { let ssa = builder.finish(); - let (acir_functions, brillig_functions) = ssa + let (acir_functions, _) = ssa .into_acir(&Brillig::default(), noirc_frontend::Distinctness::Distinct) .expect("Should compile manually written SSA into ACIR"); From 0b6f18ed59b811b68aae590dc2f653e4c06dd65e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 18:26:01 +0000 Subject: [PATCH 07/21] add some comments and rename --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index aa740d4bf7f..dd9d14cb1ea 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -58,8 +58,12 @@ struct SharedContext { /// needs to create different brillig entrypoints brillig_generated_func_pointers: BTreeMap<(FunctionId, Vec), u32>, - brillig_stdlib_func_index: HashMap, + /// Maps a Brillig std lib function (a handwritten primitive such as for inversion) -> Final generated Brillig artifact index. + /// A separate mapping from normal Brillig calls is necessary as these methods do not have an associated function id from SSA. + brillig_stdlib_func_pointer: HashMap, + /// Keeps track of Brillig std lib calls per function that need to still be resolved + /// with the correct function pointer from the `brillig_stdlib_func_pointer` map. brillig_stdlib_calls_to_resolve: HashMap>, } @@ -95,7 +99,7 @@ impl SharedContext { &self, brillig_stdlib_func: &BrilligStdlibFunc, ) -> Option<&u32> { - self.brillig_stdlib_func_index.get(brillig_stdlib_func) + self.brillig_stdlib_func_pointer.get(brillig_stdlib_func) } fn insert_generated_brillig_stdlib( @@ -106,7 +110,7 @@ impl SharedContext { opcode_location: OpcodeLocation, code: GeneratedBrillig, ) { - self.brillig_stdlib_func_index.insert(brillig_stdlib_func, generated_pointer); + self.brillig_stdlib_func_pointer.insert(brillig_stdlib_func, generated_pointer); self.add_call_to_resolve(func_id, (opcode_location, generated_pointer)); self.generated_brillig.push(code); } From c5be1c0ae8ba4fc16fae53f438463c32549ca556 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 19 Apr 2024 18:34:10 +0000 Subject: [PATCH 08/21] cargo fmt --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index dd9d14cb1ea..be45b49a9c7 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -59,10 +59,10 @@ struct SharedContext { brillig_generated_func_pointers: BTreeMap<(FunctionId, Vec), u32>, /// Maps a Brillig std lib function (a handwritten primitive such as for inversion) -> Final generated Brillig artifact index. - /// A separate mapping from normal Brillig calls is necessary as these methods do not have an associated function id from SSA. + /// A separate mapping from normal Brillig calls is necessary as these methods do not have an associated function id from SSA. brillig_stdlib_func_pointer: HashMap, - /// Keeps track of Brillig std lib calls per function that need to still be resolved + /// Keeps track of Brillig std lib calls per function that need to still be resolved /// with the correct function pointer from the `brillig_stdlib_func_pointer` map. brillig_stdlib_calls_to_resolve: HashMap>, } From a8f31544a1cb167ef6b366e4f5c58d1c23a1a192 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 22 Apr 2024 17:22:42 +0100 Subject: [PATCH 09/21] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index be45b49a9c7..e30ee6c7f51 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3049,6 +3049,9 @@ mod test { .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); + // We expect two brillig functions: + // - Quotient (shared between both divisions) + // - Inversion, caused by division-by-zero check (shared between both divisions) assert_eq!(brillig_functions.len(), 2, "Should only have generated two Brillig functions"); let main_acir = &acir_functions[0]; From 4b1e686875454fac8e6a4b1a1080aa45be5bd225 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 22 Apr 2024 17:22:54 +0100 Subject: [PATCH 10/21] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index e30ee6c7f51..d82cc2b882f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3121,6 +3121,10 @@ mod test { .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); + // We expect 3 brillig functions: + // - Quotient (shared between both divisions) + // - Inversion, caused by division-by-zero check (shared between both divisions) + // - Custom brillig function `foo` assert_eq!( brillig_functions.len(), 3, From a4c4fc3063abca79a6991ddc46833cd9e0b0f2a8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 22 Apr 2024 17:24:35 +0100 Subject: [PATCH 11/21] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index d82cc2b882f..df8caae41f4 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3018,7 +3018,7 @@ mod test { // b0(v0: u32, v1: u32, v2: u32): // v3 = div v0, v1 // constrain v3 == v2 - // v4 = div v0, v1 + // v4 = div v2, v2 // constrain v4 == u32 1 // return // } From 743ee7af62bf77c33378e7679b1d6c0b8bd802e9 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 22 Apr 2024 17:26:45 +0100 Subject: [PATCH 12/21] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index df8caae41f4..1f529ec6cc0 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3208,6 +3208,10 @@ mod test { .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions"); + // We expect 3 brillig functions: + // - Quotient (shared between both divisions) + // - Inversion, caused by division-by-zero check (shared between both divisions) + // - Custom brillig function `foo` assert_eq!( brillig_functions.len(), 3, From 79ca68dad1d7c9677156aa6120c203314906fbfe Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 22 Apr 2024 17:29:17 +0100 Subject: [PATCH 13/21] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Co-authored-by: jfecher --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 1f529ec6cc0..a65b6bed454 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -98,8 +98,8 @@ impl SharedContext { fn generated_brillig_stdlib_pointer( &self, brillig_stdlib_func: &BrilligStdlibFunc, - ) -> Option<&u32> { - self.brillig_stdlib_func_pointer.get(brillig_stdlib_func) + ) -> Option { + self.brillig_stdlib_func_pointer.get(brillig_stdlib_func).copied() } fn insert_generated_brillig_stdlib( From 737e6f516f17112d749c9638952f15e314f6d726 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 22 Apr 2024 16:29:51 +0000 Subject: [PATCH 14/21] remove deref --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a65b6bed454..c4a8dd9dcb9 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -292,7 +292,7 @@ impl Ssa { { // We cannot insert in the Brillig calls to resolve map here as we are doing an immutable borrow // to the shared context to fetch the pre-existing generated pointer. - call_to_existing_bytecode = Some((*opcode_location, *generated_pointer)); + call_to_existing_bytecode = Some((*opcode_location, generated_pointer)); } else { let code = brillig_stdlib_func.get_generated_brillig(); let generated_pointer = shared_context.new_generated_pointer(); From 417f01372f38990b8ead5e7d316bdaa2ae022880 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 22 Apr 2024 16:40:06 +0000 Subject: [PATCH 15/21] imporve entry(..).or_default() usage --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index c4a8dd9dcb9..799e43ac0f1 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -116,11 +116,7 @@ impl SharedContext { } fn add_call_to_resolve(&mut self, func_id: FunctionId, call_to_resolve: (OpcodeLocation, u32)) { - if let Some(calls_to_resolve) = self.brillig_stdlib_calls_to_resolve.get_mut(&func_id) { - calls_to_resolve.push(call_to_resolve); - } else { - self.brillig_stdlib_calls_to_resolve.insert(func_id, vec![call_to_resolve]); - } + self.brillig_stdlib_calls_to_resolve.entry(func_id).or_default().push(call_to_resolve); } } @@ -286,13 +282,13 @@ impl Ssa { for (opcode_location, brillig_stdlib_func) in &generated_acir.brillig_stdlib_func_locations { - let mut call_to_existing_bytecode = None; if let Some(generated_pointer) = shared_context.generated_brillig_stdlib_pointer(brillig_stdlib_func) { - // We cannot insert in the Brillig calls to resolve map here as we are doing an immutable borrow - // to the shared context to fetch the pre-existing generated pointer. - call_to_existing_bytecode = Some((*opcode_location, generated_pointer)); + shared_context.add_call_to_resolve( + function.id(), + (*opcode_location, generated_pointer), + ); } else { let code = brillig_stdlib_func.get_generated_brillig(); let generated_pointer = shared_context.new_generated_pointer(); @@ -304,9 +300,6 @@ impl Ssa { code, ); } - if let Some(new_call_to_resolve) = call_to_existing_bytecode { - shared_context.add_call_to_resolve(function.id(), new_call_to_resolve); - } } // Fetch the Brillig stdlib calls to resolve for this function @@ -3051,7 +3044,7 @@ mod test { assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); // We expect two brillig functions: // - Quotient (shared between both divisions) - // - Inversion, caused by division-by-zero check (shared between both divisions) + // - Inversion, caused by division-by-zero check (shared between both divisions) assert_eq!(brillig_functions.len(), 2, "Should only have generated two Brillig functions"); let main_acir = &acir_functions[0]; @@ -3123,7 +3116,7 @@ mod test { assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); // We expect 3 brillig functions: // - Quotient (shared between both divisions) - // - Inversion, caused by division-by-zero check (shared between both divisions) + // - Inversion, caused by division-by-zero check (shared between both divisions) // - Custom brillig function `foo` assert_eq!( brillig_functions.len(), @@ -3210,7 +3203,7 @@ mod test { assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions"); // We expect 3 brillig functions: // - Quotient (shared between both divisions) - // - Inversion, caused by division-by-zero check (shared between both divisions) + // - Inversion, caused by division-by-zero check (shared between both divisions) // - Custom brillig function `foo` assert_eq!( brillig_functions.len(), From 34c895cb7ad8a316987c8663da5996de32ade504 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 22 Apr 2024 16:51:06 +0000 Subject: [PATCH 16/21] cleanup brillig call resolution in into_acir --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 799e43ac0f1..a9d3529dfd6 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -95,13 +95,30 @@ impl SharedContext { self.generated_brillig.len() as u32 } - fn generated_brillig_stdlib_pointer( - &self, + fn generate_brillig_calls_to_resolve( + &mut self, brillig_stdlib_func: &BrilligStdlibFunc, - ) -> Option { - self.brillig_stdlib_func_pointer.get(brillig_stdlib_func).copied() + func_id: FunctionId, + opcode_location: OpcodeLocation, + ) { + if let Some(generated_pointer) = + self.brillig_stdlib_func_pointer.get(brillig_stdlib_func).copied() + { + self.add_call_to_resolve(func_id, (opcode_location, generated_pointer)); + } else { + let code = brillig_stdlib_func.get_generated_brillig(); + let generated_pointer = self.new_generated_pointer(); + self.insert_generated_brillig_stdlib( + *brillig_stdlib_func, + generated_pointer, + func_id, + opcode_location, + code, + ); + } } + /// Insert a newly generated Brillig stdlib function fn insert_generated_brillig_stdlib( &mut self, brillig_stdlib_func: BrilligStdlibFunc, @@ -282,24 +299,11 @@ impl Ssa { for (opcode_location, brillig_stdlib_func) in &generated_acir.brillig_stdlib_func_locations { - if let Some(generated_pointer) = - shared_context.generated_brillig_stdlib_pointer(brillig_stdlib_func) - { - shared_context.add_call_to_resolve( - function.id(), - (*opcode_location, generated_pointer), - ); - } else { - let code = brillig_stdlib_func.get_generated_brillig(); - let generated_pointer = shared_context.new_generated_pointer(); - shared_context.insert_generated_brillig_stdlib( - *brillig_stdlib_func, - generated_pointer, - function.id(), - *opcode_location, - code, - ); - } + shared_context.generate_brillig_calls_to_resolve( + brillig_stdlib_func, + function.id(), + *opcode_location, + ); } // Fetch the Brillig stdlib calls to resolve for this function From 97c2cfab6f52f6112dccdd599729eee16620e83e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 22 Apr 2024 16:52:55 +0000 Subject: [PATCH 17/21] use v2 --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a9d3529dfd6..ebf3961e018 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3030,7 +3030,7 @@ mod test { builder.insert_constrain(v0_div_v1, main_v2, None); // Call the same primitive operation again - let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + let v2_div_v2 = builder.insert_binary(main_v2, BinaryOp::Div, main_v2); let one = builder.numeric_constant(1u128, Type::unsigned(32)); builder.insert_constrain(v2_div_v2, one, None); From f71869ac3bfca1a6d024362d0177f0ab60c61c39 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 22 Apr 2024 17:06:48 +0000 Subject: [PATCH 18/21] use PLACEHOLDER_BRILLIG_INDEX --- .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 6 +++--- .../src/ssa/acir_gen/acir_ir/generated_acir.rs | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index e071e201adf..5e94f60b82a 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -1,5 +1,5 @@ use super::big_int::BigIntContext; -use super::generated_acir::{BrilligStdlibFunc, GeneratedAcir}; +use super::generated_acir::{BrilligStdlibFunc, GeneratedAcir, PLACEHOLDER_BRILLIG_INDEX}; use crate::brillig::brillig_gen::brillig_directive; use crate::brillig::brillig_ir::artifact::GeneratedBrillig; use crate::errors::{InternalError, RuntimeError, SsaReport}; @@ -333,7 +333,7 @@ impl AcirContext { vec![AcirType::field()], true, false, - 0, + PLACEHOLDER_BRILLIG_INDEX, Some(BrilligStdlibFunc::Inverse), )?; let inverted_var = Self::expect_one_var(results); @@ -723,7 +723,7 @@ impl AcirContext { vec![AcirType::unsigned(max_q_bits), AcirType::unsigned(max_rhs_bits)], true, false, - 0, + PLACEHOLDER_BRILLIG_INDEX, Some(BrilligStdlibFunc::Quotient(bit_size + 1)), )? .try_into() diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 7508347e929..ab5fa0c5245 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -24,6 +24,12 @@ use acvm::{ use iter_extended::vecmap; use num_bigint::BigUint; +/// Brillig calls such as for the Brillig std lib are resolved only after code generation is finished. +/// This index should be used when adding a Brillig call during code generation. +/// Code generation should then keep track of that unresolved call opcode which will be resolved with the +/// correct function index after code generation. +pub(crate) const PLACEHOLDER_BRILLIG_INDEX: u32 = 0; + #[derive(Debug, Default)] /// The output of the Acir-gen pass, which should only be produced for entry point Acir functions pub(crate) struct GeneratedAcir { @@ -484,7 +490,7 @@ impl GeneratedAcir { &inverse_code, inputs, outputs, - 0, + PLACEHOLDER_BRILLIG_INDEX, Some(BrilligStdlibFunc::Inverse), ); From 6f599f2ddace4c4ec31096c7704ad7701e2c1a44 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 22 Apr 2024 17:10:47 +0000 Subject: [PATCH 19/21] update tests to do v1 div v2 --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index ebf3961e018..e15a70cd391 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3015,7 +3015,7 @@ mod test { // b0(v0: u32, v1: u32, v2: u32): // v3 = div v0, v1 // constrain v3 == v2 - // v4 = div v2, v2 + // v4 = div v1, v2 // constrain v4 == u32 1 // return // } @@ -3030,9 +3030,9 @@ mod test { builder.insert_constrain(v0_div_v1, main_v2, None); // Call the same primitive operation again - let v2_div_v2 = builder.insert_binary(main_v2, BinaryOp::Div, main_v2); + let v1_div_v2 = builder.insert_binary(main_v1, BinaryOp::Div, main_v2); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - builder.insert_constrain(v2_div_v2, one, None); + builder.insert_constrain(v1_div_v2, one, None); builder.terminate_with_return(vec![]); @@ -3072,7 +3072,7 @@ mod test { // constrain v4 == v2 // v5 = call f1(v0, v1) // v6 = call f1(v0, v1) - // v7 = div v0, v1 + // v7 = div v1, v2 // constrain v7 == u32 1 // return // } @@ -3100,9 +3100,9 @@ mod test { builder.insert_call(foo, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); // Call the same primitive operation again - let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + let v1_div_v2 = builder.insert_binary(main_v1, BinaryOp::Div, main_v2); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - builder.insert_constrain(v2_div_v2, one, None); + builder.insert_constrain(v1_div_v2, one, None); builder.terminate_with_return(vec![]); @@ -3148,7 +3148,7 @@ mod test { // constrain v4 == v2 // v5 = call f1(v0, v1) // v6 = call f2(v0, v1) - // v7 = div v0, v1 + // v7 = div v1, v2 // constrain v7 == u32 1 // return // } @@ -3186,9 +3186,9 @@ mod test { builder.insert_call(bar, vec![main_v0, main_v1], vec![Type::field()]).to_vec(); // Call the same primitive operation again - let v2_div_v2 = builder.insert_binary(main_v0, BinaryOp::Div, main_v1); + let v1_div_v2 = builder.insert_binary(main_v1, BinaryOp::Div, main_v2); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - builder.insert_constrain(v2_div_v2, one, None); + builder.insert_constrain(v1_div_v2, one, None); builder.terminate_with_return(vec![]); From 33f60bf3516b807a0bd625f43896a0d79d1e98c3 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 22 Apr 2024 18:14:15 +0100 Subject: [PATCH 20/21] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index e15a70cd391..a719ec42c63 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -292,7 +292,7 @@ impl Ssa { context.convert_ssa_function(&self, function, brillig)? { // 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 do want a reference to the `SharedContext` on the generated ACIR itself, + // 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. // We then defer resolving the function IDs of those Brillig functions to when we have generated Brillig // for all normal Brillig calls. From 72b3a2c2048d9fdc85f9141c3361b6014340f5ab Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 22 Apr 2024 18:20:17 +0000 Subject: [PATCH 21/21] chore: fix merge conflict --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 9faf652cea0..ee236df8eac 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -3042,7 +3042,7 @@ mod test { // The Brillig bytecode we insert for the stdlib is hardcoded so we do not need to provide any // Brillig artifacts to the ACIR gen pass. let (acir_functions, brillig_functions) = ssa - .into_acir(&Brillig::default(), noirc_frontend::Distinctness::Distinct) + .into_acir(&Brillig::default(), noirc_frontend::ast::Distinctness::Distinct) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3114,7 +3114,7 @@ mod test { println!("{}", ssa); let (acir_functions, brillig_functions) = ssa - .into_acir(&brillig, noirc_frontend::Distinctness::Distinct) + .into_acir(&brillig, noirc_frontend::ast::Distinctness::Distinct) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3201,7 +3201,7 @@ mod test { println!("{}", ssa); let (acir_functions, brillig_functions) = ssa - .into_acir(&brillig, noirc_frontend::Distinctness::Distinct) + .into_acir(&brillig, noirc_frontend::ast::Distinctness::Distinct) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions");