From 3063bf326a1619eb51da00b9c1a570107904193f Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 12 Feb 2024 16:52:52 +0000 Subject: [PATCH] feat(avm-transpiler): implement tags for SET and others (#4545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generates tags for * `BinaryFieldOp` * `BinaryIntOp` * `Const` (SET on the AVM) * CAST on the AVM Gotcha (1): Brillig seems to only support fields of <= 126 bits. We now support CONST of a field (Brillig) and transpile it to a `SET` + `CAST`. It's very difficult to test this e2e in the `avm_test` contract since Noir does a lot of optimizations. Moreover, Noir currently generates code that compares different bit sizes and uses unsupported bit sizes. As an example ``` #[aztec(public-vm)] fn setOpcodeUint64() -> pub u64 { 1 << 60 as u64 } ``` produces (using `nargo compile --package avm_test_contract --show-ssa --print-acir`) ``` Compiled ACIR for AvmTest::avm_setOpcodeUint64 (unoptimized): current witness index : 0 public parameters indices : [] return value indices : [0] BRILLIG: inputs: [] outputs: [Simple(Witness(0))] [Const { destination: MemoryAddress(0), bit_size: 32, value: Value { inner: 1025 } }, CalldataCopy { destination_address: MemoryAddress(1024), size: 0, offset: 0 }, Call { location: 5 }, Mov { destination: MemoryAddress(1024), source: MemoryAddress(2) }, Stop { return_data_offset: 1024, return_data_size: 1 }, Const { destination: MemoryAddress(2), bit_size: 64, value: Value { inner: 2⁶⁰ } }, Mov { destination: MemoryAddress(3), source: MemoryAddress(2) }, Mov { destination: MemoryAddress(2), source: MemoryAddress(3) }, Return] Initial SSA: brillig fn avm_setOpcodeUint64 f0 { b0(): call f1() return u64 2⁶⁰ } ... After Dead Instruction Elimination: brillig fn avm_setOpcodeUint64 f0 { b0(): return u64 2⁶⁰ } ``` Closes #4268. --- avm-transpiler/src/instructions.rs | 37 ++-- avm-transpiler/src/transpile.rs | 196 +++++++++++------- avm-transpiler/src/utils.rs | 2 +- .../contracts/avm_test_contract/src/main.nr | 35 ++++ .../simulator/src/avm/avm_simulator.test.ts | 34 ++- 5 files changed, 207 insertions(+), 97 deletions(-) diff --git a/avm-transpiler/src/instructions.rs b/avm-transpiler/src/instructions.rs index bf34f64dc21..f2ab5288100 100644 --- a/avm-transpiler/src/instructions.rs +++ b/avm-transpiler/src/instructions.rs @@ -4,6 +4,7 @@ use std::fmt::{Debug, Formatter}; use crate::opcodes::AvmOpcode; /// Common values of the indirect instruction flag +pub const ALL_DIRECT: u8 = 0b00000000; pub const ZEROTH_OPERAND_INDIRECT: u8 = 0b00000001; pub const FIRST_OPERAND_INDIRECT: u8 = 0b00000010; pub const ZEROTH_FIRST_OPERANDS_INDIRECT: u8 = 0b00000011; @@ -20,10 +21,9 @@ pub struct AvmInstruction { /// The 0th bit corresponds to an instruction's 0th offset arg, 1st to 1st, etc... pub indirect: Option, - /// Some instructions have a destination or input tag - // TODO(4271): add in_tag alongside its support in TS - //pub in_tag: Option, - pub dst_tag: Option, + /// Some instructions have a destination xor input tag + /// Its usage will depend on the instruction. + pub tag: Option, /// Different instructions have different numbers of operands pub operands: Vec, @@ -35,9 +35,9 @@ impl Display for AvmInstruction { if let Some(indirect) = self.indirect { write!(f, ", indirect: {}", indirect)?; } - // TODO(4271): add in_tag alongside its support in TS - if let Some(dst_tag) = self.dst_tag { - write!(f, ", dst_tag: {}", dst_tag as u8)?; + // This will be either inTag or dstTag depending on the operation + if let Some(dst_tag) = self.tag { + write!(f, ", tag: {}", dst_tag as u8)?; } if !self.operands.is_empty() { write!(f, ", operands: [")?; @@ -58,10 +58,9 @@ impl AvmInstruction { if let Some(indirect) = self.indirect { bytes.push(indirect); } - // TODO(4271): add in_tag alongside its support in TS - if let Some(dst_tag) = self.dst_tag { - // TODO(4271): make 8 bits when TS supports deserialization of 8 bit flags - bytes.extend_from_slice(&(dst_tag as u8).to_be_bytes()); + // This will be either inTag or dstTag depending on the operation + if let Some(tag) = self.tag { + bytes.extend_from_slice(&(tag as u8).to_be_bytes()); } for operand in &self.operands { bytes.extend_from_slice(&operand.to_be_bytes()); @@ -82,14 +81,14 @@ impl Default for AvmInstruction { opcode: AvmOpcode::ADD, // TODO(4266): default to Some(0), since all instructions have indirect flag except jumps indirect: None, - dst_tag: None, + tag: None, operands: vec![], } } } /// AVM instructions may include a type tag -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub enum AvmTypeTag { UNINITIALIZED, UINT8, @@ -105,16 +104,20 @@ pub enum AvmTypeTag { /// Constants (as used by the SET instruction) can have size /// different from 32 bits pub enum AvmOperand { + U8 { value: u8 }, + U16 { value: u16 }, U32 { value: u32 }, - // TODO(4267): Support operands of size other than 32 bits (for SET) + U64 { value: u64 }, U128 { value: u128 }, } impl Display for AvmOperand { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { + AvmOperand::U8 { value } => write!(f, " U8:{}", value), + AvmOperand::U16 { value } => write!(f, " U16:{}", value), AvmOperand::U32 { value } => write!(f, " U32:{}", value), - // TODO(4267): Support operands of size other than 32 bits (for SET) + AvmOperand::U64 { value } => write!(f, " U64:{}", value), AvmOperand::U128 { value } => write!(f, " U128:{}", value), } } @@ -123,8 +126,10 @@ impl Display for AvmOperand { impl AvmOperand { pub fn to_be_bytes(&self) -> Vec { match self { + AvmOperand::U8 { value } => value.to_be_bytes().to_vec(), + AvmOperand::U16 { value } => value.to_be_bytes().to_vec(), AvmOperand::U32 { value } => value.to_be_bytes().to_vec(), - // TODO(4267): Support operands of size other than 32 bits (for SET) + AvmOperand::U64 { value } => value.to_be_bytes().to_vec(), AvmOperand::U128 { value } => value.to_be_bytes().to_vec(), } } diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs index 62dceb654c6..ad079b4b098 100644 --- a/avm-transpiler/src/transpile.rs +++ b/avm-transpiler/src/transpile.rs @@ -1,10 +1,11 @@ use acvm::acir::brillig::Opcode as BrilligOpcode; use acvm::acir::circuit::brillig::Brillig; -use acvm::brillig_vm::brillig::{BinaryFieldOp, BinaryIntOp, ValueOrArray}; +use acvm::brillig_vm::brillig::{BinaryFieldOp, BinaryIntOp, MemoryAddress, Value, ValueOrArray}; use crate::instructions::{ - AvmInstruction, AvmOperand, AvmTypeTag, FIRST_OPERAND_INDIRECT, ZEROTH_OPERAND_INDIRECT, + AvmInstruction, AvmOperand, AvmTypeTag, ALL_DIRECT, FIRST_OPERAND_INDIRECT, + ZEROTH_OPERAND_INDIRECT, }; use crate::opcodes::AvmOpcode; use crate::utils::{dbg_print_avm_program, dbg_print_brillig_program}; @@ -35,12 +36,10 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { BinaryFieldOp::Div => AvmOpcode::DIV, BinaryFieldOp::Equals => AvmOpcode::EQ, }; - // TODO(4268): set in_tag to `field` avm_instrs.push(AvmInstruction { opcode: avm_opcode, - indirect: Some(0), - // TODO(4268): TEMPORARY - typescript wireFormat expects this - dst_tag: Some(AvmTypeTag::UINT32), + indirect: Some(ALL_DIRECT), + tag: Some(AvmTypeTag::FIELD), operands: vec![ AvmOperand::U32 { value: lhs.to_usize() as u32, @@ -57,7 +56,7 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { BrilligOpcode::BinaryIntOp { destination, op, - bit_size: _, // TODO(4268): support u8..u128 and use in_tag + bit_size, lhs, rhs, } => { @@ -79,10 +78,10 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { brillig_instr ), }; - // TODO(4268): support u8..u128 and use in_tag avm_instrs.push(AvmInstruction { opcode: avm_opcode, - indirect: Some(0), + indirect: Some(ALL_DIRECT), + tag: Some(tag_from_bit_size(*bit_size)), operands: vec![ AvmOperand::U32 { value: lhs.to_usize() as u32, @@ -100,7 +99,7 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { BrilligOpcode::CalldataCopy { destination_address, size, offset } => { avm_instrs.push(AvmInstruction { opcode: AvmOpcode::CALLDATACOPY, - indirect: Some(0), + indirect: Some(ALL_DIRECT), operands: vec![ AvmOperand::U32 { value: *offset as u32, // cdOffset (calldata offset) @@ -129,7 +128,7 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { let avm_loc = brillig_pcs_to_avm_pcs[*location]; avm_instrs.push(AvmInstruction { opcode: AvmOpcode::JUMPI, - indirect: Some(0), + indirect: Some(ALL_DIRECT), operands: vec![ AvmOperand::U32 { value: avm_loc as u32, @@ -141,78 +140,26 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { ..Default::default() }); } - BrilligOpcode::Const { destination, value, bit_size:_ } => { - avm_instrs.push(AvmInstruction { - opcode: AvmOpcode::SET, - indirect: Some(0), - dst_tag: Some(AvmTypeTag::UINT128), - operands: vec![ - // TODO(4267): support u8..u128 and use dst_tag - // value - temporarily as u128 - matching wireFormat in typescript - AvmOperand::U128 { - value: value.to_usize() as u128, - }, - // dest offset - AvmOperand::U32 { - value: destination.to_usize() as u32, - }, - ], - }); + BrilligOpcode::Const { destination, value, bit_size } => { + handle_const(&mut avm_instrs, destination, value, bit_size); } BrilligOpcode::Mov { destination, source, } => { - avm_instrs.push(AvmInstruction { - opcode: AvmOpcode::MOV, - indirect: Some(0), - operands: vec![ - AvmOperand::U32 { - value: source.to_usize() as u32, - }, - AvmOperand::U32 { - value: destination.to_usize() as u32, - }, - ], - ..Default::default() - }); + avm_instrs.push(emit_mov(Some(ALL_DIRECT), source.to_usize() as u32, destination.to_usize() as u32)); } BrilligOpcode::Load { destination, source_pointer, } => { - avm_instrs.push(AvmInstruction { - opcode: AvmOpcode::MOV, - indirect: Some(ZEROTH_OPERAND_INDIRECT), // indirect srcOffset operand - operands: vec![ - AvmOperand::U32 { - value: source_pointer.to_usize() as u32, - }, - AvmOperand::U32 { - value: destination.to_usize() as u32, - }, - ], - ..Default::default() - }); + avm_instrs.push(emit_mov(Some(ZEROTH_OPERAND_INDIRECT), source_pointer.to_usize() as u32, destination.to_usize() as u32)); } BrilligOpcode::Store { destination_pointer, source, } => { - // INDIRECT dstOffset operand (bit 1 set high) - avm_instrs.push(AvmInstruction { - opcode: AvmOpcode::MOV, - indirect: Some(FIRST_OPERAND_INDIRECT), // indirect dstOffset operand - operands: vec![ - AvmOperand::U32 { - value: source.to_usize() as u32, - }, - AvmOperand::U32 { - value: destination_pointer.to_usize() as u32, - }, - ], - ..Default::default() - }); + avm_instrs.push(emit_mov(Some(FIRST_OPERAND_INDIRECT), source.to_usize() as u32, destination_pointer.to_usize() as u32)); } BrilligOpcode::Call { location } => { let avm_loc = brillig_pcs_to_avm_pcs[*location]; @@ -231,10 +178,10 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { BrilligOpcode::Stop { return_data_offset, return_data_size } => { avm_instrs.push(AvmInstruction { opcode: AvmOpcode::RETURN, - indirect: Some(0), + indirect: Some(ALL_DIRECT), operands: vec![ - AvmOperand::U32 { value: *return_data_offset as u32}, - AvmOperand::U32 { value: *return_data_size as u32}, + AvmOperand::U32 { value: *return_data_offset as u32 }, + AvmOperand::U32 { value: *return_data_size as u32 }, ], ..Default::default() }); @@ -243,12 +190,12 @@ pub fn brillig_to_avm(brillig: &Brillig) -> Vec { // TODO(https://github.com/noir-lang/noir/issues/3113): Trap should support return data avm_instrs.push(AvmInstruction { opcode: AvmOpcode::REVERT, - indirect: Some(0), + indirect: Some(ALL_DIRECT), operands: vec![ //AvmOperand::U32 { value: *return_data_offset as u32}, //AvmOperand::U32 { value: *return_data_size as u32}, - AvmOperand::U32 { value: 0}, - AvmOperand::U32 { value: 0}, + AvmOperand::U32 { value: 0 }, + AvmOperand::U32 { value: 0 }, ], ..Default::default() }); @@ -313,7 +260,7 @@ fn handle_foreign_call( avm_instrs.push(AvmInstruction { opcode, - indirect: Some(0), + indirect: Some(ALL_DIRECT), operands: vec![AvmOperand::U32 { value: dest_offset as u32, }], @@ -321,6 +268,89 @@ fn handle_foreign_call( }); } +/// Handles Brillig's CONST opcode. +fn handle_const( + avm_instrs: &mut Vec, + destination: &MemoryAddress, + value: &Value, + bit_size: &u32, +) { + let tag = tag_from_bit_size(*bit_size); + let dest = destination.to_usize() as u32; + + if !matches!(tag, AvmTypeTag::FIELD) { + avm_instrs.push(emit_set(tag, dest, value.to_u128())); + } else { + // Handling fields is a bit more complex since we cannot fit a field in a single instruction. + // We need to split the field into 128-bit chunks and set them individually. + let field = value.to_field(); + if !field.fits_in_u128() { + // If the field doesn't fit in 128 bits, we need scratch space. That's not trivial. + // Will this ever happen? ACIR supports up to 126 bit fields. + // However, it might be needed _inside_ the unconstrained function. + panic!("SET: Field value doesn't fit in 128 bits, that's not supported yet!"); + } + avm_instrs.extend([ + emit_set(AvmTypeTag::UINT128, dest, field.to_u128()), + emit_cast(dest, dest, AvmTypeTag::FIELD), + ]); + } +} + +/// Emits an AVM SET instruction. +fn emit_set(tag: AvmTypeTag, dest: u32, value: u128) -> AvmInstruction { + AvmInstruction { + opcode: AvmOpcode::SET, + indirect: Some(ALL_DIRECT), + tag: Some(tag), + operands: vec![ + // const + match tag { + AvmTypeTag::UINT8 => AvmOperand::U8 { value: value as u8 }, + AvmTypeTag::UINT16 => AvmOperand::U16 { + value: value as u16, + }, + AvmTypeTag::UINT32 => AvmOperand::U32 { + value: value as u32, + }, + AvmTypeTag::UINT64 => AvmOperand::U64 { + value: value as u64, + }, + AvmTypeTag::UINT128 => AvmOperand::U128 { value: value }, + _ => panic!("Invalid type tag {:?} for set", tag), + }, + // dest offset + AvmOperand::U32 { value: dest }, + ], + } +} + +/// Emits an AVM CAST instruction. +fn emit_cast(source: u32, destination: u32, dst_tag: AvmTypeTag) -> AvmInstruction { + AvmInstruction { + opcode: AvmOpcode::CAST, + indirect: Some(ALL_DIRECT), + tag: Some(dst_tag), + operands: vec![ + AvmOperand::U32 { value: source }, + AvmOperand::U32 { value: destination }, + ], + } +} + +/// Emits an AVM MOV instruction. +fn emit_mov(indirect: Option, source: u32, dest: u32) -> AvmInstruction { + AvmInstruction { + opcode: AvmOpcode::MOV, + indirect: indirect, + operands: vec![ + AvmOperand::U32 { value: source }, + AvmOperand::U32 { value: dest }, + ], + ..Default::default() + } +} + /// Compute an array that maps each Brillig pc to an AVM pc. /// This must be done before transpiling to properly transpile jump destinations. /// This is necessary for two reasons: @@ -339,6 +369,10 @@ fn map_brillig_pcs_to_avm_pcs(initial_offset: usize, brillig: &Brillig) -> Vec 2, BrilligOpcode::Store { .. } => 2, + BrilligOpcode::Const { bit_size, .. } => match bit_size { + 254 => 2, // Field. + _ => 1, + }, _ => 1, }; // next Brillig pc will map to an AVM pc offset by the @@ -347,3 +381,15 @@ fn map_brillig_pcs_to_avm_pcs(initial_offset: usize, brillig: &Brillig) -> Vec AvmTypeTag { + match bit_size { + 8 => AvmTypeTag::UINT8, + 16 => AvmTypeTag::UINT16, + 32 => AvmTypeTag::UINT32, + 64 => AvmTypeTag::UINT64, + 128 => AvmTypeTag::UINT128, + 254 => AvmTypeTag::FIELD, + _ => panic!("The AVM doesn't support integer bit size {:?}", bit_size), + } +} diff --git a/avm-transpiler/src/utils.rs b/avm-transpiler/src/utils.rs index e7a633e7c15..684dde7b6e4 100644 --- a/avm-transpiler/src/utils.rs +++ b/avm-transpiler/src/utils.rs @@ -11,7 +11,7 @@ use crate::instructions::AvmInstruction; /// assuming the 0th ACIR opcode is the wrapper. pub fn extract_brillig_from_acir(opcodes: &Vec) -> &Brillig { if opcodes.len() != 1 { - panic!("There should only be one brillig opcode"); + panic!("An AVM program should be contained entirely in only a single ACIR opcode flagged as 'Brillig'"); } let opcode = &opcodes[0]; match opcode { diff --git a/yarn-project/noir-contracts/contracts/avm_test_contract/src/main.nr b/yarn-project/noir-contracts/contracts/avm_test_contract/src/main.nr index 219b8e501ee..8f2d554a811 100644 --- a/yarn-project/noir-contracts/contracts/avm_test_contract/src/main.nr +++ b/yarn-project/noir-contracts/contracts/avm_test_contract/src/main.nr @@ -16,6 +16,41 @@ contract AvmTest { argA + argB } + #[aztec(public-vm)] + fn setOpcodeUint8() -> pub u8 { + 8 as u8 + } + + // Bit size 16 in Noir is deprecated. + // #[aztec(public-vm)] + // fn setOpcodeUint16() -> pub u16 { + // 60000 as u16 + // } + + #[aztec(public-vm)] + fn setOpcodeUint32() -> pub u32 { + 1 << 30 as u32 + } + + #[aztec(public-vm)] + fn setOpcodeUint64() -> pub u64 { + 1 << 60 as u64 + } + + // Can't return this since it doesn't fit in a Noir field. + // #[aztec(public-vm)] + // fn setOpcodeUint128() -> pub u128 { + // 1 << 120 as u128 + // } + + // Field should fit in 128 bits + // ACIR only supports fields of up to 126 bits! + // Same with internal fields for unconstrained functions, apprently. + #[aztec(public-vm)] + fn setOpcodeSmallField() -> pub Field { + 200 as Field + } + /************************************************************************ * AvmContext functions ************************************************************************/ diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index e2231ede872..cee68ab2283 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.test.ts @@ -11,7 +11,7 @@ import { initContext, initExecutionEnvironment, initGlobalVariables } from './fi import { Add, CalldataCopy, Return } from './opcodes/index.js'; import { encodeToBytecode } from './serialization/bytecode_serialization.js'; -describe('avm', () => { +describe('AVM simulator', () => { it('Should execute bytecode that performs basic addition', async () => { const calldata: Fr[] = [new Fr(1), new Fr(2)]; @@ -34,8 +34,7 @@ describe('avm', () => { expect(returnData).toEqual([new Fr(3)]); }); - describe('testing transpiled Noir contracts', () => { - // TODO(https://github.com/AztecProtocol/aztec-packages/issues/4361): sync wire format w/transpiler. + describe('Transpiled Noir contracts', () => { it('Should execute contract function that performs addition', async () => { const calldata: Fr[] = [new Fr(1), new Fr(2)]; @@ -53,10 +52,36 @@ describe('avm', () => { expect(results.reverted).toBe(false); const returnData = results.output; - expect(returnData.length).toBe(1); expect(returnData).toEqual([new Fr(3)]); }); + describe.each([ + ['avm_setOpcodeUint8', 8n], + // ['avm_setOpcodeUint16', 60000n], + ['avm_setOpcodeUint32', 1n << 30n], + ['avm_setOpcodeUint64', 1n << 60n], + // ['avm_setOpcodeUint128', 1n << 120n], + ['avm_setOpcodeSmallField', 200n], + ])('Should execute contract SET functions', (name: string, res: bigint) => { + it(`Should execute contract function '${name}'`, async () => { + // Decode bytecode into instructions + const artifact = AvmTestContractArtifact.functions.find(f => f.name === name)!; + const bytecode = Buffer.from(artifact.bytecode, 'base64'); + + const context = initContext(); + jest + .spyOn(context.worldState.hostStorage.contractsDb, 'getBytecode') + .mockReturnValue(Promise.resolve(bytecode)); + + const results = await new AvmSimulator(context).execute(); + + expect(results.reverted).toBe(false); + + const returnData = results.output; + expect(returnData).toEqual([new Fr(res)]); + }); + }); + describe('Test env getters from noir contract', () => { const testEnvGetter = async (valueName: string, value: any, functionName: string, globalVar: boolean = false) => { const getterArtifact = AvmTestContractArtifact.functions.find(f => f.name === functionName)!; @@ -83,7 +108,6 @@ describe('avm', () => { expect(results.reverted).toBe(false); const returnData = results.output; - expect(returnData.length).toBe(1); expect(returnData).toEqual([value.toField()]); };