feat(avm)!: variants for MOV opcode (#8440)

Yields an 11% to 21% reduction in bytecode size: ``` avm_simulation_bytecode_size_in_bytes (Token:constructor): 10,658 (-18%) avm_simulation_bytecode_size_in_bytes (FPC:constructor): 6,144 (-21%) avm_simulation_bytecode_size_in_bytes (FPC:prepare_fee): 3,089 (-21%) avm_simulation_bytecode_size_in_bytes (Token:transfer_public): 7,401 (-17%) avm_simulation_bytecode_size_in_bytes (FPC:pay_refund): 3,911 (-19%) avm_simulation_bytecode_size_in_bytes (Benchmarking:increment_balance): 2,620 (-16%) avm_simulation_bytecode_size_in_bytes (FPC:pay_refund_with_shielded_rebate): 3,911 (-19%) ``` There are some gas-related things to cleanup later.
AztecProtocol · Sep 9, 2024 · 5b27fbc · 5b27fbc
1 parent 346502c
commit 5b27fbc
Show file tree

Hide file tree

Showing 21 changed files with 283 additions and 145 deletions.
diff --git a/avm-transpiler/src/bit_traits.rs b/avm-transpiler/src/bit_traits.rs
@@ -0,0 +1,68 @@
+use acvm::{AcirField, FieldElement};
+
+fn get_msb(n: u128) -> usize {
+    let mut n = n;
+    let mut msb = 0;
+    while n > 0 {
+        n >>= 1;
+        msb += 1;
+    }
+    msb
+}
+
+pub trait BitsQueryable {
+    fn num_bits(&self) -> usize;
+}
+
+impl BitsQueryable for FieldElement {
+    fn num_bits(&self) -> usize {
+        AcirField::num_bits(self) as usize
+    }
+}
+
+impl BitsQueryable for u8 {
+    fn num_bits(&self) -> usize {
+        get_msb(*self as u128)
+    }
+}
+
+impl BitsQueryable for u16 {
+    fn num_bits(&self) -> usize {
+        get_msb(*self as u128)
+    }
+}
+
+impl BitsQueryable for u32 {
+    fn num_bits(&self) -> usize {
+        get_msb(*self as u128)
+    }
+}
+
+impl BitsQueryable for u64 {
+    fn num_bits(&self) -> usize {
+        get_msb(*self as u128)
+    }
+}
+
+impl BitsQueryable for u128 {
+    fn num_bits(&self) -> usize {
+        get_msb(*self)
+    }
+}
+
+pub fn bits_needed_for<T: BitsQueryable>(val: &T) -> usize {
+    let num_bits = val.num_bits();
+    if num_bits < 8 {
+        8
+    } else if num_bits < 16 {
+        16
+    } else if num_bits < 32 {
+        32
+    } else if num_bits < 64 {
+        64
+    } else if num_bits < 128 {
+        128
+    } else {
+        254
+    }
+}
diff --git a/avm-transpiler/src/main.rs b/avm-transpiler/src/main.rs
@@ -6,6 +6,7 @@ use std::env;
 use std::fs;
 use std::path::Path;
 
+mod bit_traits;
 mod instructions;
 mod opcodes;
 mod transpile;

diff --git a/avm-transpiler/src/opcodes.rs b/avm-transpiler/src/opcodes.rs
@@ -1,6 +1,6 @@
 /// All AVM opcodes
 /// Keep updated with TS, cpp, and docs protocol specs!
-#[allow(clippy::upper_case_acronyms, dead_code)]
+#[allow(clippy::upper_case_acronyms, dead_code, non_camel_case_types)]
 #[derive(PartialEq, Copy, Clone, Debug, Eq, Hash)]
 pub enum AvmOpcode {
     // Compute
@@ -42,7 +42,8 @@ pub enum AvmOpcode {
     INTERNALRETURN,
     // Memory
     SET,
-    MOV,
+    MOV_8,
+    MOV_16,
     CMOV,
     // World state
     SLOAD,
@@ -129,7 +130,8 @@ impl AvmOpcode {
             AvmOpcode::INTERNALRETURN => "INTERNALRETURN",
             // Machine State - Memory
             AvmOpcode::SET => "SET",
-            AvmOpcode::MOV => "MOV",
+            AvmOpcode::MOV_8 => "MOV_8",
+            AvmOpcode::MOV_16 => "MOV_16",
             AvmOpcode::CMOV => "CMOV",
 
             // World State

diff --git a/avm-transpiler/src/transpile.rs b/avm-transpiler/src/transpile.rs
@@ -9,12 +9,13 @@ use acvm::brillig_vm::brillig::{
 use acvm::{AcirField, FieldElement};
 use noirc_errors::debug_info::DebugInfo;
 
+use crate::bit_traits::bits_needed_for;
 use crate::instructions::{
     AvmInstruction, AvmOperand, AvmTypeTag, ALL_DIRECT, FIRST_OPERAND_INDIRECT,
     SECOND_OPERAND_INDIRECT, ZEROTH_OPERAND_INDIRECT,
 };
 use crate::opcodes::AvmOpcode;
-use crate::utils::{dbg_print_avm_program, dbg_print_brillig_program};
+use crate::utils::{dbg_print_avm_program, dbg_print_brillig_program, make_operand};
 
 /// Transpile a Brillig program to AVM bytecode
 pub fn brillig_to_avm(
@@ -216,7 +217,7 @@ pub fn brillig_to_avm(
     // We are adding a MOV instruction that moves a value to itself.
     // This should therefore not affect the program's execution.
     avm_instrs.push(AvmInstruction {
-        opcode: AvmOpcode::MOV,
+        opcode: AvmOpcode::MOV_8,
         indirect: Some(ALL_DIRECT),
         operands: vec![AvmOperand::U32 { value: 0x18ca }, AvmOperand::U32 { value: 0x18ca }],
         ..Default::default()
@@ -741,10 +742,18 @@ fn generate_cast_instruction(
 
 /// Generates an AVM MOV instruction.
 fn generate_mov_instruction(indirect: Option<u8>, source: u32, dest: u32) -> AvmInstruction {
+    let bits_needed = [source, dest].iter().map(bits_needed_for).max().unwrap();
+
+    let mov_opcode = match bits_needed {
+        8 => AvmOpcode::MOV_8,
+        16 => AvmOpcode::MOV_16,
+        _ => panic!("MOV operands must fit in 16 bits but needed {}", bits_needed),
+    };
+
     AvmInstruction {
-        opcode: AvmOpcode::MOV,
+        opcode: mov_opcode,
         indirect,
-        operands: vec![AvmOperand::U32 { value: source }, AvmOperand::U32 { value: dest }],
+        operands: vec![make_operand(bits_needed, &source), make_operand(bits_needed, &dest)],
         ..Default::default()
     }
 }

diff --git a/avm-transpiler/src/utils.rs b/avm-transpiler/src/utils.rs
@@ -7,7 +7,7 @@ use log::{debug, info, trace};
 use acvm::acir::brillig::Opcode as BrilligOpcode;
 use acvm::acir::circuit::{AssertionPayload, Opcode, Program};
 
-use crate::instructions::AvmInstruction;
+use crate::instructions::{AvmInstruction, AvmOperand};
 use crate::opcodes::AvmOpcode;
 
 /// Extract the Brillig program from its `Program` wrapper.
@@ -90,3 +90,14 @@ pub fn dbg_print_avm_program(avm_program: &[AvmInstruction]) {
         debug!("\t{0:?}: {1}", opcode, count);
     }
 }
+
+pub fn make_operand<T: Into<u128> + Copy>(bits: usize, value: &T) -> AvmOperand {
+    match bits {
+        8 => AvmOperand::U8 { value: Into::<u128>::into(*value) as u8 },
+        16 => AvmOperand::U16 { value: Into::<u128>::into(*value) as u16 },
+        32 => AvmOperand::U32 { value: Into::<u128>::into(*value) as u32 },
+        64 => AvmOperand::U64 { value: Into::<u128>::into(*value) as u64 },
+        128 => AvmOperand::U128 { value: Into::<u128>::into(*value) },
+        _ => panic!("Invalid operand size for bits: {}", bits),
+    }
+}
diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp
@@ -587,10 +587,10 @@ TEST_F(AvmExecutionTests, movOpcode)
                                "01"                       // U8
                                "13"                       // val 19
                                "000000AB"                 // dst_offset 171
-                               + to_hex(OpCode::MOV) +    // opcode MOV
+                               + to_hex(OpCode::MOV_8) +  // opcode MOV
                                "00"                       // Indirect flag
-                               "000000AB"                 // src_offset 171
-                               "00000021"                 // dst_offset 33
+                               "AB"                       // src_offset 171
+                               "21"                       // dst_offset 33
                                + to_hex(OpCode::RETURN) + // opcode RETURN
                                "00"                       // Indirect flag
                                "00000000"                 // ret offset 0
@@ -613,9 +613,9 @@ TEST_F(AvmExecutionTests, movOpcode)
     // MOV
     EXPECT_THAT(
         instructions.at(1),
-        AllOf(Field(&Instruction::op_code, OpCode::MOV),
+        AllOf(Field(&Instruction::op_code, OpCode::MOV_8),
               Field(&Instruction::operands,
-                    ElementsAre(VariantWith<uint8_t>(0), VariantWith<uint32_t>(171), VariantWith<uint32_t>(33)))));
+                    ElementsAre(VariantWith<uint8_t>(0), VariantWith<uint8_t>(171), VariantWith<uint8_t>(33)))));
 
     auto trace = gen_trace_from_instr(instructions);
 
@@ -701,10 +701,10 @@ TEST_F(AvmExecutionTests, indMovOpcode)
                                "01"                       // U8
                                "FF"                       // val 255
                                "0000000A"                 // dst_offset 10
-                               + to_hex(OpCode::MOV) +    // opcode MOV
+                               + to_hex(OpCode::MOV_8) +  // opcode MOV
                                "01"                       // Indirect flag
-                               "00000001"                 // src_offset 1 --> direct offset 10
-                               "00000002"                 // dst_offset 2 --> direct offset 11
+                               "01"                       // src_offset 1 --> direct offset 10
+                               "02"                       // dst_offset 2 --> direct offset 11
                                + to_hex(OpCode::RETURN) + // opcode RETURN
                                "00"                       // Indirect flag
                                "00000000"                 // ret offset 0
@@ -717,9 +717,9 @@ TEST_F(AvmExecutionTests, indMovOpcode)
 
     // MOV
     EXPECT_THAT(instructions.at(3),
-                AllOf(Field(&Instruction::op_code, OpCode::MOV),
+                AllOf(Field(&Instruction::op_code, OpCode::MOV_8),
                       Field(&Instruction::operands,
-                            ElementsAre(VariantWith<uint8_t>(1), VariantWith<uint32_t>(1), VariantWith<uint32_t>(2)))));
+                            ElementsAre(VariantWith<uint8_t>(1), VariantWith<uint8_t>(1), VariantWith<uint8_t>(2)))));
 
     auto trace = gen_trace_from_instr(instructions);
 

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/deserialization.cpp
@@ -88,7 +88,8 @@ const std::unordered_map<OpCode, std::vector<OperandType>> OPCODE_WIRE_FORMAT =
 
     // Machine State - Memory
     // OpCode::SET is handled differently
-    { OpCode::MOV, { OperandType::INDIRECT, OperandType::UINT32, OperandType::UINT32 } },
+    { OpCode::MOV_8, { OperandType::INDIRECT, OperandType::UINT8, OperandType::UINT8 } },
+    { OpCode::MOV_16, { OperandType::INDIRECT, OperandType::UINT16, OperandType::UINT16 } },
     { OpCode::CMOV,
       { OperandType::INDIRECT, OperandType::UINT32, OperandType::UINT32, OperandType::UINT32, OperandType::UINT32 } },
 

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp
@@ -616,10 +616,15 @@ std::vector<Row> Execution::gen_trace(std::vector<Instruction> const& instructio
                 std::get<uint8_t>(inst.operands.at(0)), val, std::get<uint32_t>(inst.operands.at(3)), in_tag);
             break;
         }
-        case OpCode::MOV:
+        case OpCode::MOV_8:
             trace_builder.op_mov(std::get<uint8_t>(inst.operands.at(0)),
-                                 std::get<uint32_t>(inst.operands.at(1)),
-                                 std::get<uint32_t>(inst.operands.at(2)));
+                                 std::get<uint8_t>(inst.operands.at(1)),
+                                 std::get<uint8_t>(inst.operands.at(2)));
+            break;
+        case OpCode::MOV_16:
+            trace_builder.op_mov(std::get<uint8_t>(inst.operands.at(0)),
+                                 std::get<uint16_t>(inst.operands.at(1)),
+                                 std::get<uint16_t>(inst.operands.at(2)));
             break;
         case OpCode::CMOV:
             trace_builder.op_cmov(std::get<uint8_t>(inst.operands.at(0)),

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/fixed_gas.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/fixed_gas.cpp
@@ -52,7 +52,8 @@ const std::unordered_map<OpCode, FixedGasTable::GasRow> GAS_COST_TABLE = {
     { OpCode::INTERNALCALL, make_cost(AVM_INTERNALCALL_BASE_L2_GAS, 0, AVM_INTERNALCALL_DYN_L2_GAS, 0) },
     { OpCode::INTERNALRETURN, make_cost(AVM_INTERNALRETURN_BASE_L2_GAS, 0, AVM_INTERNALRETURN_DYN_L2_GAS, 0) },
     { OpCode::SET, make_cost(AVM_SET_BASE_L2_GAS, 0, AVM_SET_DYN_L2_GAS, 0) },
-    { OpCode::MOV, make_cost(AVM_MOV_BASE_L2_GAS, 0, AVM_MOV_DYN_L2_GAS, 0) },
+    { OpCode::MOV_8, make_cost(AVM_MOV_BASE_L2_GAS, 0, AVM_MOV_DYN_L2_GAS, 0) },
+    { OpCode::MOV_16, make_cost(AVM_MOV_BASE_L2_GAS, 0, AVM_MOV_DYN_L2_GAS, 0) },
     { OpCode::CMOV, make_cost(AVM_CMOV_BASE_L2_GAS, 0, AVM_CMOV_DYN_L2_GAS, 0) },
     { OpCode::SLOAD, make_cost(AVM_SLOAD_BASE_L2_GAS, 0, AVM_SLOAD_DYN_L2_GAS, 0) },
     { OpCode::SSTORE, make_cost(AVM_SSTORE_BASE_L2_GAS, 0, AVM_SSTORE_DYN_L2_GAS, 0) },

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/opcode.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/opcode.cpp
@@ -104,8 +104,10 @@ std::string to_string(OpCode opcode)
     // Machine State - Memory
     case OpCode::SET:
         return "SET";
-    case OpCode::MOV:
-        return "MOV";
+    case OpCode::MOV_8:
+        return "MOV_8";
+    case OpCode::MOV_16:
+        return "MOV_16";
     case OpCode::CMOV:
         return "CMOV";
     // World State

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/opcode.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/opcode.hpp
@@ -68,7 +68,8 @@ enum class OpCode : uint8_t {
     INTERNALRETURN,
     // Machine State - Memory
     SET,
-    MOV,
+    MOV_8,
+    MOV_16,
     CMOV,
 
     // World State

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
@@ -1817,7 +1817,8 @@ void AvmTraceBuilder::op_mov(uint8_t indirect, uint32_t src_offset, uint32_t dst
     mem_trace_builder.write_into_memory(call_ptr, clk, IntermRegister::IC, direct_dst_offset, val, tag, tag);
 
     // Constrain gas cost
-    gas_trace_builder.constrain_gas(clk, OpCode::MOV);
+    // FIXME: not great that we are having to choose one specific opcode here!
+    gas_trace_builder.constrain_gas(clk, OpCode::MOV_8);
 
     main_trace.push_back(Row{
         .main_clk = clk,

diff --git a/yarn-project/simulator/src/avm/avm_gas.ts b/yarn-project/simulator/src/avm/avm_gas.ts
@@ -91,7 +91,8 @@ const BaseGasCosts: Record<Opcode, Gas> = {
   [Opcode.INTERNALCALL]: makeCost(c.AVM_INTERNALCALL_BASE_L2_GAS, 0),
   [Opcode.INTERNALRETURN]: makeCost(c.AVM_INTERNALRETURN_BASE_L2_GAS, 0),
   [Opcode.SET]: makeCost(c.AVM_SET_BASE_L2_GAS, 0),
-  [Opcode.MOV]: makeCost(c.AVM_MOV_BASE_L2_GAS, 0),
+  [Opcode.MOV_8]: makeCost(c.AVM_MOV_BASE_L2_GAS, 0),
+  [Opcode.MOV_16]: makeCost(c.AVM_MOV_BASE_L2_GAS, 0),
   [Opcode.CMOV]: makeCost(c.AVM_CMOV_BASE_L2_GAS, 0),
   [Opcode.SLOAD]: makeCost(c.AVM_SLOAD_BASE_L2_GAS, 0),
   [Opcode.SSTORE]: makeCost(c.AVM_SSTORE_BASE_L2_GAS, 0),
@@ -156,7 +157,8 @@ const DynamicGasCosts: Record<Opcode, Gas> = {
   [Opcode.INTERNALCALL]: makeCost(c.AVM_INTERNALCALL_DYN_L2_GAS, 0),
   [Opcode.INTERNALRETURN]: makeCost(c.AVM_INTERNALRETURN_DYN_L2_GAS, 0),
   [Opcode.SET]: makeCost(c.AVM_SET_DYN_L2_GAS, 0),
-  [Opcode.MOV]: makeCost(c.AVM_MOV_DYN_L2_GAS, 0),
+  [Opcode.MOV_8]: makeCost(c.AVM_MOV_DYN_L2_GAS, 0),
+  [Opcode.MOV_16]: makeCost(c.AVM_MOV_DYN_L2_GAS, 0),
   [Opcode.CMOV]: makeCost(c.AVM_CMOV_DYN_L2_GAS, 0),
   [Opcode.SLOAD]: makeCost(c.AVM_SLOAD_DYN_L2_GAS, 0),
   [Opcode.SSTORE]: makeCost(c.AVM_SSTORE_DYN_L2_GAS, 0),

diff --git a/yarn-project/simulator/src/avm/bytecode_utils.ts b/yarn-project/simulator/src/avm/bytecode_utils.ts
@@ -1,10 +1,10 @@
 import { promisify } from 'util';
 import { gunzip } from 'zlib';
 
-import { Mov } from '../avm/opcodes/memory.js';
+import { Opcode } from './serialization/instruction_serialization.js';
 
 const AVM_MAGIC_SUFFIX = Buffer.from([
-  Mov.opcode, // opcode
+  Opcode.MOV_8, // opcode
   0x00, // indirect
   ...Buffer.from('000018ca', 'hex'), // srcOffset
   ...Buffer.from('000018ca', 'hex'), // dstOffset