Skip to content

Commit

Permalink
fix: Don't reuse brillig with slice arguments (#5800)
Browse files Browse the repository at this point in the history
This is a quick fix after
#5737 since that PR
assumes that one generated brillig is valid for all calls to the brillig
function. This is however not true, since the generated brillig can
differ if the arguments contains slices. This is because the entry point
codegen depends on the size of the slice.
We currently cannot copy slices of any length into brillig due to the
limitations of
[CALLDATACOPY](https://yp-aztec.netlify.app/docs/public-vm/instruction-set#calldatacopy)
where the size being copied needs to be known at compile-time. I'm going
to chat with the AVM team to see if we can lift this restriction and
make brillig entry points be able to copy in arguments that contain
slices of any length.
  • Loading branch information
sirasistant authored Apr 17, 2024
1 parent 03e693a commit be9f24c
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 13 deletions.
2 changes: 1 addition & 1 deletion l1-contracts/src/core/libraries/ConstantsGen.sol
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ library Constants {
uint256 internal constant DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE =
0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631;
uint256 internal constant DEPLOYER_CONTRACT_ADDRESS =
0x2d8e7aedc70b65d49e6aa0794d8d12721896c177e87126701f6e60d184358e74;
0x0b98aeb0111208b95d8d71f484f849d7ab44b3e34c545d13736a707ce3cb0839;
uint256 internal constant L1_TO_L2_MESSAGE_ORACLE_CALL_LENGTH = 17;
uint256 internal constant MAX_NOTE_FIELDS_LENGTH = 20;
uint256 internal constant GET_NOTE_ORACLE_RETURN_LENGTH = 23;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ global REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUE = 0xe7af8166354
// CONTRACT INSTANCE CONSTANTS
// sha224sum 'struct ContractInstanceDeployed'
global DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631;
global DEPLOYER_CONTRACT_ADDRESS = 0x2d8e7aedc70b65d49e6aa0794d8d12721896c177e87126701f6e60d184358e74;
global DEPLOYER_CONTRACT_ADDRESS = 0x0b98aeb0111208b95d8d71f484f849d7ab44b3e34c545d13736a707ce3cb0839;

// NOIR CONSTANTS - constants used only in yarn-packages/noir-contracts
// Some are defined here because Noir doesn't yet support globals referencing other globals yet.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::collections::{BTreeMap, HashMap};
use crate::ssa::ir::dfg::CallStack;

/// Represents a parameter or a return value of an entry point function.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)]
pub(crate) enum BrilligParameter {
/// A single address parameter or return value. Holds the bit size of the parameter.
SingleAddr(u32),
Expand Down
30 changes: 20 additions & 10 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,18 @@ struct SharedContext {
/// There can be Brillig functions specified in SSA which do not act as
/// entry points in ACIR (e.g. only called by other Brillig functions)
/// This mapping is necessary to use the correct function pointer for a Brillig call.
brillig_generated_func_pointers: BTreeMap<FunctionId, u32>,
/// 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<BrilligParameter>), u32>,
}

impl SharedContext {
fn generated_brillig_pointer(&self, func_id: &FunctionId) -> Option<&u32> {
self.brillig_generated_func_pointers.get(func_id)
fn generated_brillig_pointer(
&self,
func_id: FunctionId,
arguments: Vec<BrilligParameter>,
) -> Option<&u32> {
self.brillig_generated_func_pointers.get(&(func_id, arguments))
}

fn generated_brillig(&self, func_pointer: usize) -> &GeneratedBrillig {
Expand All @@ -67,10 +73,11 @@ impl SharedContext {
fn insert_generated_brillig(
&mut self,
func_id: FunctionId,
arguments: Vec<BrilligParameter>,
generated_pointer: u32,
code: GeneratedBrillig,
) {
self.brillig_generated_func_pointers.insert(func_id, generated_pointer);
self.brillig_generated_func_pointers.insert((func_id, arguments), generated_pointer);
self.generated_brillig.push(code);
}

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

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

// 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 All @@ -370,7 +377,7 @@ impl<'a> Context<'a> {
// We are guaranteed to have a Brillig function pointer of `0` as main itself is marked as unconstrained
0,
)?;
self.shared_context.insert_generated_brillig(main_func.id(), 0, code);
self.shared_context.insert_generated_brillig(main_func.id(), arguments, 0, code);

let output_vars: Vec<_> = output_values
.iter()
Expand Down Expand Up @@ -657,15 +664,17 @@ impl<'a> Context<'a> {
}
}
let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg));
let arguments = self.gen_brillig_parameters(arguments, dfg);

let outputs: Vec<AcirType> = vecmap(result_ids, |result_id| {
dfg.type_of_value(*result_id).into()
});

// Check whether we have already generated Brillig for this function
// If we have, re-use the generated code to set-up the Brillig call.
let output_values = if let Some(generated_pointer) =
self.shared_context.generated_brillig_pointer(id)
let output_values = if let Some(generated_pointer) = self
.shared_context
.generated_brillig_pointer(*id, arguments.clone())
{
let code = self
.shared_context
Expand All @@ -680,8 +689,8 @@ impl<'a> Context<'a> {
*generated_pointer,
)?
} else {
let arguments = self.gen_brillig_parameters(arguments, dfg);
let code = self.gen_brillig_for(func, arguments, brillig)?;
let code =
self.gen_brillig_for(func, arguments.clone(), brillig)?;
let generated_pointer =
self.shared_context.new_generated_pointer();
let output_values = self.acir_context.brillig_call(
Expand All @@ -695,6 +704,7 @@ impl<'a> Context<'a> {
)?;
self.shared_context.insert_generated_brillig(
*id,
arguments,
generated_pointer,
code,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ fn main() {
y: 8,
}
]);
let brillig_sum = sum_slice(slice);
assert_eq(brillig_sum, 55);

slice = slice.push_back([
Point {
x: 15,
Expand Down

0 comments on commit be9f24c

Please sign in to comment.