Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement references in brillig #1901

Merged
merged 2 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// The features being tested is brillig calls passing arrays around
fn main(x: [u32; 3]) {
assert(entry_point(x) == 9);
another_entry_point(x);
}

unconstrained fn inner(x : [u32; 3]) -> [u32; 3] {
Expand All @@ -14,3 +15,19 @@ unconstrained fn entry_point(x : [u32; 3]) -> u32 {
y[0] + y[1] + y[2]
}

unconstrained fn nested_fn_that_allocates(value: u32) -> u32 {
let x = [value, value, value];
let y = inner(x);
y[0] + y[1] + y[2]
}

unconstrained fn another_entry_point(x: [u32; 3]) {
assert(x[0] == 1);
assert(x[1] == 2);
assert(x[2] == 3);
assert(nested_fn_that_allocates(1) == 6);
// x should be unchanged
assert(x[0] == 1);
assert(x[1] == 2);
assert(x[2] == 3);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
authors = [""]
compiler_version = "0.5.1"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x = "2"
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
unconstrained fn main(mut x: Field) {
add1(&mut x);
assert(x == 3);

// https://github.com/noir-lang/noir/issues/1899
// let mut s = S { y: x };
// s.add2();
// assert(s.y == 5);

// Test that normal mutable variables are still copied
let mut a = 0;
mutate_copy(a);
assert(a == 0);

// Test something 3 allocations deep
let mut nested_allocations = Nested { y: &mut &mut 0 };
add1(*nested_allocations.y);
assert(**nested_allocations.y == 1);

// Test nested struct allocations with a mutable reference to an array.
let mut c = C {
foo: 0,
bar: &mut C2 {
array: &mut [1, 2],
},
};
*c.bar.array = [3, 4];
let arr: [Field; 2] = *c.bar.array;
assert(arr[0] == 3);
assert(arr[1] == 4);
}

unconstrained fn add1(x: &mut Field) {
*x += 1;
}

struct S { y: Field }

struct Nested { y: &mut &mut Field }

struct C {
foo: Field,
bar: &mut C2,
}

struct C2 {
array: &mut [Field; 2]
}

impl S {
unconstrained fn add2(&mut self) {
self.y += 2;
}
}

unconstrained fn mutate_copy(mut a: Field) {
a = 7;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x = "2"
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
fn main() {
let mut x = 2;
fn main(mut x: Field) {
add1(&mut x);
assert(x == 3);

let mut s = S { y: x };
s.add2();
assert(s.y == 5);
// https://github.com/noir-lang/noir/issues/1899
// let mut s = S { y: x };
// s.add2();
// assert(s.y == 5);

// Test that normal mutable variables are still copied
let mut a = 0;
Expand Down
35 changes: 21 additions & 14 deletions crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
// Simple parameters and arrays are passed as already filled registers
// In the case of arrays, the values should already be in memory and the register should
// Be a valid pointer to the array.
Type::Numeric(_) | Type::Array(..) => {
Type::Numeric(_) | Type::Array(..) | Type::Reference => {
self.function_context.get_or_create_register(self.brillig_context, *param_id);
}
_ => {
Expand All @@ -169,24 +169,25 @@
self.brillig_context.constrain_instruction(condition);
}
Instruction::Allocate => {
let value: crate::ssa_refactor::ir::map::Id<Value> =
dfg.instruction_results(instruction_id)[0];
self.function_context.get_or_create_register(self.brillig_context, value);
let value = dfg.instruction_results(instruction_id)[0];
let address_register =
self.function_context.get_or_create_register(self.brillig_context, value);
self.brillig_context.allocate_instruction(address_register);
}
Instruction::Store { address, value } => {
let target_register = self.convert_ssa_value(*address, dfg);
let address_register = self.convert_ssa_value(*address, dfg);
let source_register = self.convert_ssa_value(*value, dfg);

self.brillig_context.mov_instruction(target_register, source_register);
self.brillig_context.store_instruction(address_register, source_register);
}
Instruction::Load { address } => {
let target_register = self.function_context.get_or_create_register(
self.brillig_context,
dfg.instruction_results(instruction_id)[0],
);
let source_register = self.convert_ssa_value(*address, dfg);
let address_register = self.convert_ssa_value(*address, dfg);

self.brillig_context.mov_instruction(target_register, source_register);
self.brillig_context.load_instruction(target_register, address_register);
}
Instruction::Not(value) => {
let condition = self.convert_ssa_value(*value, dfg);
Expand Down Expand Up @@ -353,7 +354,7 @@
);
}
}
_ => unimplemented!("ICE: Value {:?} not storeable in memory", value),

Check warning on line 357 in crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (storeable)
}
}

Expand Down Expand Up @@ -497,6 +498,18 @@
/// This probably should be explicitly casted in SSA to avoid having to coerce at this level.
pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type {
match (lhs_type, rhs_type) {
(_, Type::Function) | (Type::Function, _) => {
unreachable!("Functions are invalid in binary operations")
}
(_, Type::Reference) | (Type::Reference, _) => {
unreachable!("References are invalid in binary operations")
sirasistant marked this conversation as resolved.
Show resolved Hide resolved
}
(_, Type::Array(..)) | (Type::Array(..), _) => {
unreachable!("Arrays are invalid in binary operations")
}
(_, Type::Slice(..)) | (Type::Slice(..), _) => {
unreachable!("Arrays are invalid in binary operations")
}
// If either side is a Field constant then, we coerce into the type
// of the other operand
(Type::Numeric(NumericType::NativeField), typ)
Expand All @@ -510,12 +523,6 @@
);
Type::Numeric(lhs_type)
}
(lhs_type, rhs_type) => {
unreachable!(
"ICE: Binary operation between types {:?} and {:?} is not allowed",
lhs_type, rhs_type
)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl FunctionContext {
.map(|&value_id| {
let typ = func.dfg.type_of_value(value_id);
match typ {
Type::Numeric(_) => BrilligParameter::Register,
Type::Numeric(_) | Type::Reference => BrilligParameter::Register,
Type::Array(..) => BrilligParameter::HeapArray(compute_size_of_type(&typ)),
_ => unimplemented!("Unsupported function parameter type {typ:?}"),
}
Expand All @@ -75,7 +75,7 @@ impl FunctionContext {
.map(|&value_id| {
let typ = func.dfg.type_of_value(value_id);
match typ {
Type::Numeric(_) => BrilligParameter::Register,
Type::Numeric(_) | Type::Reference => BrilligParameter::Register,
Type::Array(..) => BrilligParameter::HeapArray(compute_size_of_type(&typ)),
_ => unimplemented!("Unsupported return value type {typ:?}"),
}
Expand Down
45 changes: 41 additions & 4 deletions crates/noirc_evaluator/src/brillig/brillig_ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,16 @@ pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 64;
pub(crate) enum ReservedRegisters {
/// This register stores the stack pointer. Allocations must be done after this pointer.
StackPointer = 0,
/// This register stores the previous stack pointer. The registers of the caller are stored here.
PreviousStackPointer = 1,
}

impl ReservedRegisters {
/// The number of reserved registers.
///
/// This is used to offset the general registers
/// which should not overwrite the special register
const NUM_RESERVED_REGISTERS: usize = 1;
const NUM_RESERVED_REGISTERS: usize = 2;

/// Returns the length of the reserved registers
pub(crate) fn len() -> usize {
Expand All @@ -59,6 +61,11 @@ impl ReservedRegisters {
RegisterIndex::from(ReservedRegisters::StackPointer as usize)
}

/// Returns the previous stack pointer register. This will be used to restore the registers after a fn call.
pub(crate) fn previous_stack_pointer() -> RegisterIndex {
RegisterIndex::from(ReservedRegisters::PreviousStackPointer as usize)
}

/// Returns a user defined (non-reserved) register index.
fn user_register_index(index: usize) -> RegisterIndex {
RegisterIndex::from(index + ReservedRegisters::len())
Expand Down Expand Up @@ -135,6 +142,24 @@ impl BrilligContext {
});
}

/// Allocates a single value and stores the
/// pointer to the array in `pointer_register`
pub(crate) fn allocate_instruction(&mut self, pointer_register: RegisterIndex) {
debug_show::allocate_instruction(pointer_register);
let size_register = self.make_constant(1_u128.into());
self.push_opcode(BrilligOpcode::Mov {
destination: pointer_register,
source: ReservedRegisters::stack_pointer(),
});
self.push_opcode(BrilligOpcode::BinaryIntOp {
destination: ReservedRegisters::stack_pointer(),
op: BinaryIntOp::Add,
bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
lhs: ReservedRegisters::stack_pointer(),
rhs: size_register,
});
}

/// Gets the value in the array at index `index` and stores it in `result`
pub(crate) fn array_get(
&mut self,
Expand Down Expand Up @@ -618,12 +643,21 @@ impl BrilligContext {
//
// Note that here it is important that the stack pointer register is at register 0,
// as after the first register save we add to the pointer.
let used_registers: Vec<_> = self.registers.used_registers_iter().collect();
let mut used_registers: Vec<_> = self.registers.used_registers_iter().collect();

// Also dump the previous stack pointer
used_registers.push(ReservedRegisters::previous_stack_pointer());
for register in used_registers.iter() {
self.store_instruction(ReservedRegisters::stack_pointer(), *register);
// Add one to our stack pointer
self.usize_op(ReservedRegisters::stack_pointer(), BinaryIntOp::Add, 1);
}

// Store the location of our registers in the previous stack pointer
self.mov_instruction(
ReservedRegisters::previous_stack_pointer(),
ReservedRegisters::stack_pointer(),
);
used_registers
}

Expand All @@ -632,10 +666,13 @@ impl BrilligContext {
// Load all of the used registers that we saved.
// We do all the reverse operations of save_all_used_registers.
// Iterate our registers in reverse
let iterator_register = self.allocate_register();
self.mov_instruction(iterator_register, ReservedRegisters::previous_stack_pointer());

for register in used_registers.iter().rev() {
// Subtract one from our stack pointer
self.usize_op(ReservedRegisters::stack_pointer(), BinaryIntOp::Sub, 1);
self.load_instruction(*register, ReservedRegisters::stack_pointer());
self.usize_op(iterator_register, BinaryIntOp::Sub, 1);
self.load_instruction(*register, iterator_register);
}
}

Expand Down
7 changes: 7 additions & 0 deletions crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ impl DebugToString for RegisterIndex {
fn debug_to_string(&self) -> String {
if *self == ReservedRegisters::stack_pointer() {
"Stack".into()
} else if *self == ReservedRegisters::previous_stack_pointer() {
"PrevStack".into()
} else {
format!("R{}", self.to_usize())
}
Expand Down Expand Up @@ -214,6 +216,11 @@ pub(crate) fn allocate_array_instruction(
debug_println!(" ALLOCATE_ARRAY {} SIZE {}", pointer_register, size_register);
}

/// Debug function for allocate_instruction
pub(crate) fn allocate_instruction(pointer_register: RegisterIndex) {
debug_println!(" ALLOCATE {} ", pointer_register);
}

/// Debug function for array_get
pub(crate) fn array_get(array_ptr: RegisterIndex, index: RegisterIndex, result: RegisterIndex) {
debug_println!(" ARRAY_GET {}[{}] -> {}", array_ptr, index, result);
Expand Down