Skip to content

Commit

Permalink
fix: entrypoint register allocation
Browse files Browse the repository at this point in the history
  • Loading branch information
@sirasistant @fcarreiro
sirasistant authored and fcarreiro committed Sep 6, 2024
1 parent 9de462a commit 963bc66
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 94 deletions.
29 changes: 12 additions & 17 deletions noir/noir-repo/acvm-repo/acir/tests/test_program_serialization.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ fn complex_brillig_foreign_call() {
value: FieldElement::from(0_usize),
},
brillig::Opcode::CalldataCopy {
destination_address: MemoryAddress(0),
destination_address: MemoryAddress(32),
size_address: MemoryAddress(0),
offset_address: MemoryAddress(1),
},
Expand All @@ -271,11 +271,6 @@ fn complex_brillig_foreign_call() {
bit_size: BitSize::Integer(IntegerBitSize::U32),
value: FieldElement::from(3_usize),
},
brillig::Opcode::CalldataCopy {
destination_address: MemoryAddress(0),
size_address: MemoryAddress(0),
offset_address: MemoryAddress(1),
},
brillig::Opcode::CalldataCopy {
destination_address: MemoryAddress(1),
size_address: MemoryAddress(3),
Expand Down Expand Up @@ -343,17 +338,17 @@ fn complex_brillig_foreign_call() {

let bytes = Program::serialize_program(&program);
let expected_serialization: Vec<u8> = vec![
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 85, 93, 14, 194, 32, 12, 110, 97, 34, 209, 55, 79,
96, 162, 7, 96, 122, 1, 239, 98, 124, 211, 232, 163, 199, 223, 200, 74, 214, 117, 100, 123,
88, 73, 182, 47, 33, 45, 208, 54, 253, 5, 132, 14, 190, 93, 72, 252, 142, 168, 33, 26, 207,
45, 12, 145, 100, 31, 68, 195, 50, 212, 168, 103, 43, 148, 242, 209, 108, 192, 71, 91, 192,
71, 96, 245, 95, 97, 189, 53, 235, 162, 154, 63, 155, 153, 159, 10, 186, 249, 114, 180,
223, 19, 245, 50, 209, 81, 201, 192, 120, 240, 6, 72, 221, 115, 144, 86, 232, 174, 210,
139, 230, 206, 95, 8, 101, 219, 193, 77, 196, 86, 46, 166, 91, 240, 34, 225, 138, 49, 213,
158, 236, 149, 168, 197, 84, 190, 28, 227, 121, 215, 69, 28, 161, 239, 184, 231, 247, 243,
123, 191, 254, 86, 168, 115, 181, 136, 11, 227, 175, 226, 30, 133, 158, 212, 229, 224, 159,
73, 78, 22, 51, 114, 115, 182, 79, 237, 58, 19, 159, 62, 173, 6, 242, 203, 5, 245, 209, 6,
0, 0,
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 85, 81, 14, 194, 48, 8, 133, 118, 206, 26, 255, 60,
129, 137, 30, 160, 211, 11, 120, 23, 227, 159, 70, 63, 61, 190, 146, 209, 140, 177, 46,
251, 24, 77, 182, 151, 44, 116, 45, 16, 120, 64, 139, 208, 34, 252, 63, 228, 245, 134, 165,
99, 73, 251, 30, 250, 72, 186, 55, 150, 113, 30, 26, 180, 243, 21, 75, 197, 232, 86, 16,
163, 47, 16, 35, 136, 250, 47, 176, 222, 150, 117, 49, 229, 207, 103, 230, 167, 130, 118,
190, 106, 254, 223, 178, 12, 154, 104, 50, 114, 48, 28, 188, 30, 82, 247, 236, 180, 23, 62,
171, 236, 178, 185, 202, 27, 194, 216, 119, 36, 54, 142, 35, 185, 149, 203, 233, 18, 131,
34, 220, 48, 167, 38, 176, 191, 18, 181, 168, 5, 63, 178, 179, 8, 123, 232, 186, 234, 254,
126, 125, 158, 143, 175, 87, 148, 74, 51, 194, 73, 172, 207, 234, 28, 149, 157, 182, 149,
144, 15, 70, 78, 23, 51, 122, 83, 190, 15, 208, 181, 70, 122, 152, 126, 56, 83, 244, 10,
181, 6, 0, 0,
];

assert_eq!(bytes, expected_serialization)
Expand Down
16 changes: 8 additions & 8 deletions noir/noir-repo/acvm-repo/acvm_js/test/shared/complex_foreign_call.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { WitnessMap } from '@noir-lang/acvm_js';

// See `complex_brillig_foreign_call` integration test in `acir/tests/test_program_serialization.rs`.
export const bytecode = Uint8Array.from([
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 85, 93, 14, 194, 32, 12, 110, 97, 34, 209, 55, 79, 96, 162, 7, 96, 122, 1,
239, 98, 124, 211, 232, 163, 199, 223, 200, 74, 214, 117, 100, 123, 88, 73, 182, 47, 33, 45, 208, 54, 253, 5, 132, 14,
190, 93, 72, 252, 142, 168, 33, 26, 207, 45, 12, 145, 100, 31, 68, 195, 50, 212, 168, 103, 43, 148, 242, 209, 108,
192, 71, 91, 192, 71, 96, 245, 95, 97, 189, 53, 235, 162, 154, 63, 155, 153, 159, 10, 186, 249, 114, 180, 223, 19,
245, 50, 209, 81, 201, 192, 120, 240, 6, 72, 221, 115, 144, 86, 232, 174, 210, 139, 230, 206, 95, 8, 101, 219, 193,
77, 196, 86, 46, 166, 91, 240, 34, 225, 138, 49, 213, 158, 236, 149, 168, 197, 84, 190, 28, 227, 121, 215, 69, 28,
161, 239, 184, 231, 247, 243, 123, 191, 254, 86, 168, 115, 181, 136, 11, 227, 175, 226, 30, 133, 158, 212, 229, 224,
159, 73, 78, 22, 51, 114, 115, 182, 79, 237, 58, 19, 159, 62, 173, 6, 242, 203, 5, 245, 209, 6, 0, 0,
31, 139, 8, 0, 0, 0, 0, 0, 0, 255, 213, 85, 81, 14, 194, 48, 8, 133, 118, 206, 26, 255, 60, 129, 137, 30, 160, 211,
11, 120, 23, 227, 159, 70, 63, 61, 190, 146, 209, 140, 177, 46, 251, 24, 77, 182, 151, 44, 116, 45, 16, 120, 64, 139,
208, 34, 252, 63, 228, 245, 134, 165, 99, 73, 251, 30, 250, 72, 186, 55, 150, 113, 30, 26, 180, 243, 21, 75, 197, 232,
86, 16, 163, 47, 16, 35, 136, 250, 47, 176, 222, 150, 117, 49, 229, 207, 103, 230, 167, 130, 118, 190, 106, 254, 223,
178, 12, 154, 104, 50, 114, 48, 28, 188, 30, 82, 247, 236, 180, 23, 62, 171, 236, 178, 185, 202, 27, 194, 216, 119,
36, 54, 142, 35, 185, 149, 203, 233, 18, 131, 34, 220, 48, 167, 38, 176, 191, 18, 181, 168, 5, 63, 178, 179, 8, 123,
232, 186, 234, 254, 126, 125, 158, 143, 175, 87, 148, 74, 51, 194, 73, 172, 207, 234, 28, 149, 157, 182, 149, 144, 15,
70, 78, 23, 51, 122, 83, 190, 15, 208, 181, 70, 122, 152, 126, 56, 83, 244, 10, 181, 6, 0, 0,
]);
export const initialWitnessMap: WitnessMap = new Map([
[1, '0x0000000000000000000000000000000000000000000000000000000000000001'],
Expand Down
106 changes: 55 additions & 51 deletions noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ impl<F: AcirField + DebugToString> BrilligContext<F, Stack> {
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);

Expand All @@ -64,90 +67,91 @@ impl<F: AcirField + DebugToString> BrilligContext<F, Stack> {
// 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);
self.usize_const_instruction(
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<BrilligVariable> {
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(
Expand Down
36 changes: 30 additions & 6 deletions noir/noir-repo/tooling/debugger/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand All @@ -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!(
Expand All @@ -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)),
})
);
Expand Down
13 changes: 5 additions & 8 deletions yarn-project/simulator/src/avm/opcodes/external_calls.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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),
]),
Expand Down
8 changes: 4 additions & 4 deletions yarn-project/simulator/src/avm/opcodes/memory.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);

Expand Down

0 comments on commit 963bc66

Please sign in to comment.