diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls_array/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls_array/src/main.nr index ebe37a9b006..39b2ad1582c 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls_array/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls_array/src/main.nr @@ -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] { @@ -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); +} \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/Nargo.toml new file mode 100644 index 00000000000..09a8dd8e8c8 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.5.1" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/Prover.toml new file mode 100644 index 00000000000..151faa5a9b1 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/Prover.toml @@ -0,0 +1 @@ +x = "2" \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/src/main.nr new file mode 100644 index 00000000000..46372940bc8 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_references/src/main.nr @@ -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; +} diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml index e69de29bb2d..151faa5a9b1 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/references/Prover.toml @@ -0,0 +1 @@ +x = "2" \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr index 6e5eddd8057..10d8cd25152 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/references/src/main.nr @@ -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; diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 7f8eabcf27f..8b836ccf874 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -142,7 +142,7 @@ impl<'block> BrilligBlock<'block> { // 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); } _ => { @@ -169,24 +169,25 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.constrain_instruction(condition); } Instruction::Allocate => { - let value: crate::ssa_refactor::ir::map::Id = - 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); @@ -497,6 +498,18 @@ impl<'block> BrilligBlock<'block> { /// 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") + } + (_, 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) @@ -510,12 +523,6 @@ pub(crate) fn type_of_binary_operation(lhs_type: Type, rhs_type: Type) -> Type { ); Type::Numeric(lhs_type) } - (lhs_type, rhs_type) => { - unreachable!( - "ICE: Binary operation between types {:?} and {:?} is not allowed", - lhs_type, rhs_type - ) - } } } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index a501e9117a2..27cce0a83f9 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -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:?}"), } @@ -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:?}"), } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index e25291202a1..47aee7b89a1 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -40,6 +40,8 @@ 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 { @@ -47,7 +49,7 @@ impl ReservedRegisters { /// /// 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 { @@ -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()) @@ -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, @@ -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 } @@ -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); } } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index 37c0f4006f3..1e2836fb9bd 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -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()) } @@ -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);