Skip to content

Commit be9f24c

Browse files
authored
fix: Don't reuse brillig with slice arguments (#5800)
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.
1 parent 03e693a commit be9f24c

File tree

5 files changed

+26
-13
lines changed
  • l1-contracts/src/core/libraries
  • noir-projects/noir-protocol-circuits/crates/types/src
  • noir/noir-repo
    • compiler/noirc_evaluator/src
    • test_programs/execution_success/brillig_slice_input/src

5 files changed

+26
-13
lines changed

l1-contracts/src/core/libraries/ConstantsGen.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ library Constants {
8383
uint256 internal constant DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE =
8484
0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631;
8585
uint256 internal constant DEPLOYER_CONTRACT_ADDRESS =
86-
0x2d8e7aedc70b65d49e6aa0794d8d12721896c177e87126701f6e60d184358e74;
86+
0x0b98aeb0111208b95d8d71f484f849d7ab44b3e34c545d13736a707ce3cb0839;
8787
uint256 internal constant L1_TO_L2_MESSAGE_ORACLE_CALL_LENGTH = 17;
8888
uint256 internal constant MAX_NOTE_FIELDS_LENGTH = 20;
8989
uint256 internal constant GET_NOTE_ORACLE_RETURN_LENGTH = 23;

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ global REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUE = 0xe7af8166354
118118
// CONTRACT INSTANCE CONSTANTS
119119
// sha224sum 'struct ContractInstanceDeployed'
120120
global DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE = 0x85864497636cf755ae7bde03f267ce01a520981c21c3682aaf82a631;
121-
global DEPLOYER_CONTRACT_ADDRESS = 0x2d8e7aedc70b65d49e6aa0794d8d12721896c177e87126701f6e60d184358e74;
121+
global DEPLOYER_CONTRACT_ADDRESS = 0x0b98aeb0111208b95d8d71f484f849d7ab44b3e34c545d13736a707ce3cb0839;
122122

123123
// NOIR CONSTANTS - constants used only in yarn-packages/noir-contracts
124124
// Some are defined here because Noir doesn't yet support globals referencing other globals yet.

noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use std::collections::{BTreeMap, HashMap};
44
use crate::ssa::ir::dfg::CallStack;
55

66
/// Represents a parameter or a return value of an entry point function.
7-
#[derive(Debug, Clone)]
7+
#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)]
88
pub(crate) enum BrilligParameter {
99
/// A single address parameter or return value. Holds the bit size of the parameter.
1010
SingleAddr(u32),

noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,18 @@ struct SharedContext {
5252
/// There can be Brillig functions specified in SSA which do not act as
5353
/// entry points in ACIR (e.g. only called by other Brillig functions)
5454
/// This mapping is necessary to use the correct function pointer for a Brillig call.
55-
brillig_generated_func_pointers: BTreeMap<FunctionId, u32>,
55+
/// This uses the brillig parameters in the map since using slices with different lengths
56+
/// needs to create different brillig entrypoints
57+
brillig_generated_func_pointers: BTreeMap<(FunctionId, Vec<BrilligParameter>), u32>,
5658
}
5759

5860
impl SharedContext {
59-
fn generated_brillig_pointer(&self, func_id: &FunctionId) -> Option<&u32> {
60-
self.brillig_generated_func_pointers.get(func_id)
61+
fn generated_brillig_pointer(
62+
&self,
63+
func_id: FunctionId,
64+
arguments: Vec<BrilligParameter>,
65+
) -> Option<&u32> {
66+
self.brillig_generated_func_pointers.get(&(func_id, arguments))
6167
}
6268

6369
fn generated_brillig(&self, func_pointer: usize) -> &GeneratedBrillig {
@@ -67,10 +73,11 @@ impl SharedContext {
6773
fn insert_generated_brillig(
6874
&mut self,
6975
func_id: FunctionId,
76+
arguments: Vec<BrilligParameter>,
7077
generated_pointer: u32,
7178
code: GeneratedBrillig,
7279
) {
73-
self.brillig_generated_func_pointers.insert(func_id, generated_pointer);
80+
self.brillig_generated_func_pointers.insert((func_id, arguments), generated_pointer);
7481
self.generated_brillig.push(code);
7582
}
7683

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

359-
let code = self.gen_brillig_for(main_func, arguments, brillig)?;
366+
let code = self.gen_brillig_for(main_func, arguments.clone(), brillig)?;
360367

361368
// We specifically do not attempt execution of the brillig code being generated as this can result in it being
362369
// replaced with constraints on witnesses to the program outputs.
@@ -370,7 +377,7 @@ impl<'a> Context<'a> {
370377
// We are guaranteed to have a Brillig function pointer of `0` as main itself is marked as unconstrained
371378
0,
372379
)?;
373-
self.shared_context.insert_generated_brillig(main_func.id(), 0, code);
380+
self.shared_context.insert_generated_brillig(main_func.id(), arguments, 0, code);
374381

375382
let output_vars: Vec<_> = output_values
376383
.iter()
@@ -657,15 +664,17 @@ impl<'a> Context<'a> {
657664
}
658665
}
659666
let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg));
667+
let arguments = self.gen_brillig_parameters(arguments, dfg);
660668

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

665673
// Check whether we have already generated Brillig for this function
666674
// If we have, re-use the generated code to set-up the Brillig call.
667-
let output_values = if let Some(generated_pointer) =
668-
self.shared_context.generated_brillig_pointer(id)
675+
let output_values = if let Some(generated_pointer) = self
676+
.shared_context
677+
.generated_brillig_pointer(*id, arguments.clone())
669678
{
670679
let code = self
671680
.shared_context
@@ -680,8 +689,8 @@ impl<'a> Context<'a> {
680689
*generated_pointer,
681690
)?
682691
} else {
683-
let arguments = self.gen_brillig_parameters(arguments, dfg);
684-
let code = self.gen_brillig_for(func, arguments, brillig)?;
692+
let code =
693+
self.gen_brillig_for(func, arguments.clone(), brillig)?;
685694
let generated_pointer =
686695
self.shared_context.new_generated_pointer();
687696
let output_values = self.acir_context.brillig_call(
@@ -695,6 +704,7 @@ impl<'a> Context<'a> {
695704
)?;
696705
self.shared_context.insert_generated_brillig(
697706
*id,
707+
arguments,
698708
generated_pointer,
699709
code,
700710
);

noir/noir-repo/test_programs/execution_success/brillig_slice_input/src/main.nr

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ fn main() {
2525
y: 8,
2626
}
2727
]);
28+
let brillig_sum = sum_slice(slice);
29+
assert_eq(brillig_sum, 55);
30+
2831
slice = slice.push_back([
2932
Point {
3033
x: 15,

0 commit comments

Comments
 (0)