Skip to content

Commit

Permalink
fix: Don't reuse brillig with slice arguments (AztecProtocol/aztec-pa…
Browse files Browse the repository at this point in the history
…ckages#5800)

This is a quick fix after
AztecProtocol/aztec-packages#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
AztecBot committed Apr 17, 2024
2 parents 8314554 + 2aae5ce commit 961ca28
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 12 deletions.
2 changes: 1 addition & 1 deletion .aztec-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3fb94c0cd5ffba20a99b97c0088ae5ef357c205d
be9f24c16484b26a1eb88bcf35b785553160995d
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 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 961ca28

Please sign in to comment.