diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 5ee00cf6c29..c85940cc1c7 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -46,6 +46,9 @@ impl BrilligContext { arguments: &[BrilligParameter], return_parameters: &[BrilligParameter], ) { + // We need to allocate the variable for every argument first so any register allocation doesn't mangle the expected order. + let mut argument_variables = self.allocate_function_arguments(arguments); + let calldata_size = Self::flattened_tuple_size(arguments); let return_data_size = Self::flattened_tuple_size(return_parameters); @@ -64,75 +67,45 @@ impl BrilligContext { // Copy calldata self.copy_and_cast_calldata(arguments); - // Allocate the variables for every argument: let mut current_calldata_pointer = Self::calldata_start_offset(); - let mut argument_variables: Vec<_> = arguments - .iter() - .map(|argument| match argument { - BrilligParameter::SingleAddr(bit_size) => { - let single_address = self.allocate_register(); - let var = BrilligVariable::SingleAddr(SingleAddrVariable { - address: single_address, - bit_size: *bit_size, - }); - self.mov_instruction(single_address, MemoryAddress(current_calldata_pointer)); - current_calldata_pointer += 1; - var - } - BrilligParameter::Array(_, _) => { - let pointer_to_the_array_in_calldata = - self.make_usize_constant_instruction(current_calldata_pointer.into()); - let rc_register = self.make_usize_constant_instruction(1_usize.into()); - let flattened_size = Self::flattened_size(argument); - let var = BrilligVariable::BrilligArray(BrilligArray { - pointer: pointer_to_the_array_in_calldata.address, - size: flattened_size, - rc: rc_register.address, - }); - - current_calldata_pointer += flattened_size; - var - } - BrilligParameter::Slice(_, _) => { - let pointer_to_the_array_in_calldata = - self.make_usize_constant_instruction(current_calldata_pointer.into()); - - let flattened_size = Self::flattened_size(argument); - let size_register = self.make_usize_constant_instruction(flattened_size.into()); - let rc_register = self.make_usize_constant_instruction(1_usize.into()); - - let var = BrilligVariable::BrilligVector(BrilligVector { - pointer: pointer_to_the_array_in_calldata.address, - size: size_register.address, - rc: rc_register.address, - }); - - current_calldata_pointer += flattened_size; - var - } - }) - .collect(); - - // Deflatten arrays + // Initialize the variables with the calldata for (argument_variable, argument) in argument_variables.iter_mut().zip(arguments) { match (argument_variable, argument) { + (BrilligVariable::SingleAddr(single_address), BrilligParameter::SingleAddr(_)) => { + self.mov_instruction( + single_address.address, + MemoryAddress(current_calldata_pointer), + ); + current_calldata_pointer += 1; + } ( BrilligVariable::BrilligArray(array), BrilligParameter::Array(item_type, item_count), ) => { + let flattened_size = array.size; + self.usize_const_instruction(array.pointer, current_calldata_pointer.into()); + self.usize_const_instruction(array.rc, 1_usize.into()); + + // Deflatten the array let deflattened_address = self.deflatten_array(item_type, array.size, array.pointer); self.mov_instruction(array.pointer, deflattened_address); array.size = item_type.len() * item_count; self.deallocate_register(deflattened_address); + + current_calldata_pointer += flattened_size; } ( BrilligVariable::BrilligVector(vector), BrilligParameter::Slice(item_type, item_count), ) => { let flattened_size = Self::flattened_size(argument); + self.usize_const_instruction(vector.pointer, current_calldata_pointer.into()); + self.usize_const_instruction(vector.rc, 1_usize.into()); + self.usize_const_instruction(vector.size, flattened_size.into()); + // Deflatten the vector let deflattened_address = self.deflatten_array(item_type, flattened_size, vector.pointer); self.mov_instruction(vector.pointer, deflattened_address); @@ -140,14 +113,45 @@ impl BrilligContext { vector.size, (item_type.len() * item_count).into(), ); - self.deallocate_register(deflattened_address); + + current_calldata_pointer += flattened_size; } - _ => {} + _ => unreachable!("ICE: cannot match variables against arguments"), } } } + fn allocate_function_arguments( + &mut self, + arguments: &[BrilligParameter], + ) -> Vec { + arguments + .iter() + .map(|argument| match argument { + BrilligParameter::SingleAddr(bit_size) => { + BrilligVariable::SingleAddr(SingleAddrVariable { + address: self.allocate_register(), + bit_size: *bit_size, + }) + } + BrilligParameter::Array(_, _) => { + let flattened_size = Self::flattened_size(argument); + BrilligVariable::BrilligArray(BrilligArray { + pointer: self.allocate_register(), + size: flattened_size, + rc: self.allocate_register(), + }) + } + BrilligParameter::Slice(_, _) => BrilligVariable::BrilligVector(BrilligVector { + pointer: self.allocate_register(), + size: self.allocate_register(), + rc: self.allocate_register(), + }), + }) + .collect() + } + fn copy_and_cast_calldata(&mut self, arguments: &[BrilligParameter]) { let calldata_size = Self::flattened_tuple_size(arguments); self.calldata_copy_instruction( diff --git a/noir/noir-repo/tooling/debugger/src/context.rs b/noir/noir-repo/tooling/debugger/src/context.rs index feae1dda42d..dde3fe84d88 100644 --- a/noir/noir-repo/tooling/debugger/src/context.rs +++ b/noir/noir-repo/tooling/debugger/src/context.rs @@ -1046,7 +1046,7 @@ mod tests { }) ); - // Execute the first Brillig opcode (calldata copy) + // Const let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( @@ -1058,7 +1058,7 @@ mod tests { }) ); - // execute the second Brillig opcode (const) + // Const let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( @@ -1070,26 +1070,50 @@ mod tests { }) ); - // try to execute the third Brillig opcode (and resolve the foreign call) + // Calldatacopy let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 2 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); - // retry the third Brillig opcode (foreign call should be finished) + // Const let result = context.step_into_opcode(); assert!(matches!(result, DebugCommandResult::Ok)); assert_eq!( context.get_current_debug_location(), Some(DebugLocation { circuit_id: 0, - opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 4 }, + brillig_function_id: Some(BrilligFunctionId(0)), + }) + ); + + // try to execute the Brillig opcode (and resolve the foreign call) + let result = context.step_into_opcode(); + assert!(matches!(result, DebugCommandResult::Ok)); + assert_eq!( + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 4 }, + brillig_function_id: Some(BrilligFunctionId(0)), + }) + ); + + // retry the Brillig opcode (foreign call should be finished) + let result = context.step_into_opcode(); + assert!(matches!(result, DebugCommandResult::Ok)); + assert_eq!( + context.get_current_debug_location(), + Some(DebugLocation { + circuit_id: 0, + opcode_location: OpcodeLocation::Brillig { acir_index: 0, brillig_index: 5 }, brillig_function_id: Some(BrilligFunctionId(0)), }) ); diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts index 663b079a438..48f703c8185 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -4,7 +4,7 @@ import { mock } from 'jest-mock-extended'; import { type PublicSideEffectTraceInterface } from '../../public/side_effect_trace_interface.js'; import { type AvmContext } from '../avm_context.js'; -import { Field, Uint8, Uint32 } from '../avm_memory_types.js'; +import { Field, TypeTag, Uint8, Uint32 } from '../avm_memory_types.js'; import { markBytecodeAsAvm } from '../bytecode_utils.js'; import { adjustCalldataIndex, initContext, initHostStorage, initPersistableStateManager } from '../fixtures/index.js'; import { type HostStorage } from '../journal/host_storage.js'; @@ -14,7 +14,7 @@ import { mockGetBytecode, mockTraceFork } from '../test_utils.js'; import { L2GasLeft } from './context_getters.js'; import { Call, Return, Revert, StaticCall } from './external_calls.js'; import { type Instruction } from './instruction.js'; -import { CalldataCopy } from './memory.js'; +import { CalldataCopy, Set } from './memory.js'; import { SStore } from './storage.js'; describe('External Calls', () => { @@ -83,12 +83,9 @@ describe('External Calls', () => { // const otherContextInstructionsL2GasCost = 780; // Includes the cost of the call itself const otherContextInstructionsBytecode = markBytecodeAsAvm( encodeToBytecode([ - new CalldataCopy( - /*indirect=*/ 0, - /*csOffset=*/ adjustCalldataIndex(0), - /*copySize=*/ argsSize, - /*dstOffset=*/ 0, - ), + new Set(/*indirect=*/ 0, TypeTag.UINT32, adjustCalldataIndex(0), /*dstOffset=*/ 0), + new Set(/*indirect=*/ 0, TypeTag.UINT32, argsSize, /*dstOffset=*/ 1), + new CalldataCopy(/*indirect=*/ 0, /*csOffsetAddress=*/ 0, /*copySizeOffset=*/ 1, /*dstOffset=*/ 0), new SStore(/*indirect=*/ 0, /*srcOffset=*/ valueOffset, /*slotOffset=*/ slotOffset), new Return(/*indirect=*/ 0, /*retOffset=*/ 0, /*size=*/ 2), ]), diff --git a/yarn-project/simulator/src/avm/opcodes/memory.test.ts b/yarn-project/simulator/src/avm/opcodes/memory.test.ts index efbbdf5a362..798994b765c 100644 --- a/yarn-project/simulator/src/avm/opcodes/memory.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/memory.test.ts @@ -432,14 +432,14 @@ describe('Memory instructions', () => { const buf = Buffer.from([ CalldataCopy.opcode, // opcode 0x01, // indirect - ...Buffer.from('12345678', 'hex'), // cdOffset - ...Buffer.from('23456789', 'hex'), // copysize + ...Buffer.from('12345678', 'hex'), // cdOffsetAddress + ...Buffer.from('23456789', 'hex'), // copysizeOffset ...Buffer.from('3456789a', 'hex'), // dstOffset ]); const inst = new CalldataCopy( /*indirect=*/ 0x01, - /*cdOffset=*/ 0x12345678, - /*copysize=*/ 0x23456789, + /*cdOffsetAddress=*/ 0x12345678, + /*copysizeOffset=*/ 0x23456789, /*dstOffset=*/ 0x3456789a, );