fix(avm): alu interface

barretenberg/cpp/pil/vm2/alu.pil

@@ -5,12 +5,9 @@ pol commit ia;
 pol commit ib;
 pol commit ic;
 pol commit op;
-pol commit ia_addr;
-pol commit ib_addr;
-pol commit dst_addr;
 
 #[SEL_ADD_BINARY]
 sel_op_add * (1 - sel_op_add) = 0;
 
 #[ALU_ADD]
-ia + ib = ic;
+ia + ib = ic;

barretenberg/cpp/src/barretenberg/vm2/generated/flavor_variables.hpp

@@ -45,10 +45,10 @@ namespace bb::avm2 {
 
 struct AvmFlavorVariables {
     static constexpr size_t NUM_PRECOMPUTED_ENTITIES = 45;
-    static constexpr size_t NUM_WITNESS_ENTITIES = 903;
+    static constexpr size_t NUM_WITNESS_ENTITIES = 900;
     static constexpr size_t NUM_SHIFTED_ENTITIES = 135;
     static constexpr size_t NUM_WIRES = NUM_WITNESS_ENTITIES + NUM_PRECOMPUTED_ENTITIES;
-    static constexpr size_t NUM_ALL_ENTITIES = 1083;
+    static constexpr size_t NUM_ALL_ENTITIES = 1080;
 
     // Need to be templated for recursive verifier
     template <typename FF_>

barretenberg/cpp/src/barretenberg/vm2/simulation/alu.cpp

@@ -5,25 +5,16 @@
 
 namespace bb::avm2::simulation {
 
-void Alu::add(ContextInterface& context, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr)
+FF Alu::add(const ValueRefAndTag& a, const ValueRefAndTag& b)
 {
-    auto& memory = context.get_memory();
-
     // TODO: check types and tags and propagate.
-    auto a = memory.get(a_addr);
-    auto b = memory.get(b_addr);
-    // TODO: handle different types, wrapping, etc.
-    auto c = a.value + b.value;
-    memory.set(dst_addr, c, a.tag);
+    // TODO(ilyas): need big switch here for different types, wrapping
+    // TODO(ilyas): come up with a better way than a big switch
+    FF c = a.value + b.value;
 
     // TODO: add tags to events.
-    events.emit({ .operation = AluOperation::ADD,
-                  .a_addr = a_addr,
-                  .b_addr = b_addr,
-                  .dst_addr = dst_addr,
-                  .a = a.value,
-                  .b = b.value,
-                  .res = c });
+    events.emit({ .operation = AluOperation::ADD, .a = a.value, .b = b.value, .c = c });
+    return c;
 }
 
-} // namespace bb::avm2::simulation
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation/alu.hpp

@@ -14,7 +14,8 @@ namespace bb::avm2::simulation {
 class AluInterface {
   public:
     virtual ~AluInterface() = default;
-    virtual void add(ContextInterface&, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr) = 0;
+    // I'd like to return a ValueRefAndTag, but the MemoryValue& doesnt live long enough.
+    virtual FF add(const ValueRefAndTag& a, const ValueRefAndTag& b) = 0;
 };
 
 class Alu : public AluInterface {
@@ -23,11 +24,10 @@ class Alu : public AluInterface {
         : events(event_emitter)
     {}
 
-    // Operands are expected to be direct.
-    void add(ContextInterface& context, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr) override;
+    FF add(const ValueRefAndTag& a, const ValueRefAndTag& b) override;
 
   private:
     EventEmitterInterface<AluEvent>& events;
 };
 
-} // namespace bb::avm2::simulation
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation/alu.test.cpp

@@ -12,33 +12,18 @@
 namespace bb::avm2::simulation {
 namespace {
 
-using ::testing::ReturnRef;
-using ::testing::StrictMock;
-
 TEST(AvmSimulationAluTest, Add)
 {
-    NoopEventEmitter<MemoryEvent> emitter;
-    Memory mem(/*space_id=*/0, emitter);
-    StrictMock<MockContext> context;
-    EXPECT_CALL(context, get_memory()).WillRepeatedly(ReturnRef(mem));
-
     EventEmitter<AluEvent> alu_event_emitter;
     Alu alu(alu_event_emitter);
 
-    // TODO: actually can choose to mock, not even use a memory, check the events, etc.
-    MemoryAddress a_addr = 0;
-    MemoryAddress b_addr = 1;
-    MemoryAddress dst_addr = 2;
-
-    mem.set(a_addr, 1, MemoryTag::U32);
-    mem.set(b_addr, 2, MemoryTag::U32);
+    ValueRefAndTag a = { .value = 1, .tag = MemoryTag::U32 };
+    ValueRefAndTag b = { .value = 2, .tag = MemoryTag::U32 };
 
-    alu.add(context, a_addr, b_addr, dst_addr);
+    FF c = alu.add(a, b);
 
-    auto c = mem.get(dst_addr);
-    EXPECT_EQ(c.value, 3);
-    EXPECT_EQ(c.tag, MemoryTag::U32);
+    EXPECT_EQ(c, 3);
 }
 
 } // namespace
-} // namespace bb::avm2::simulation
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation/events/alu_event.hpp

@@ -14,12 +14,16 @@ enum class AluOperation {
 
 struct AluEvent {
     AluOperation operation;
-    MemoryAddress a_addr;
-    MemoryAddress b_addr;
-    MemoryAddress dst_addr;
     MemoryValue a;
     MemoryValue b;
-    MemoryValue res;
+    MemoryValue c;
+    // Only need single tag info here (check this for MOV or CAST )
+    // For operations that have a specific output tag (e.g., EQ/LT), the output tag is unambiguous
+    // We still might prefer to include tags per operands to simply tracegen...
+    MemoryTag tag;
+    // To be used with deduplicating event emitters.
+    using Key = std::tuple<AluOperation, MemoryValue, MemoryValue, MemoryValue, MemoryTag>;
+    Key get_key() const { return { operation, a, b, c, tag }; }
 };
 
-} // namespace bb::avm2::simulation
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation/execution.cpp

@@ -15,7 +15,11 @@ namespace bb::avm2::simulation {
 
 void Execution::add(ContextInterface& context, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr)
 {
-    alu.add(context, a_addr, b_addr, dst_addr);
+    auto& memory = context.get_memory();
+    ValueRefAndTag a = memory.get(a_addr);
+    ValueRefAndTag b = memory.get(b_addr);
+    FF c = alu.add(a, b);
+    memory.set(dst_addr, c, a.tag);
 }
 
 // TODO: My dispatch system makes me have a uint8_t tag. Rethink.

barretenberg/cpp/src/barretenberg/vm2/simulation/execution.test.cpp

@@ -46,7 +46,13 @@ class ExecutionSimulationTest : public ::testing::Test {
 
 TEST_F(ExecutionSimulationTest, Add)
 {
-    EXPECT_CALL(alu, add(Ref(context), 4, 5, 6));
+    ValueRefAndTag a = { .value = 4, .tag = MemoryTag::U32 };
+    ValueRefAndTag b = { .value = 5, .tag = MemoryTag::U32 };
+
+    EXPECT_CALL(context, get_memory);
+    EXPECT_CALL(memory, get).Times(2).WillOnce(Return(a)).WillOnce(Return(b));
+    EXPECT_CALL(alu, add(a, b)).WillOnce(Return(9));
+    EXPECT_CALL(memory, set(6, FF(9), MemoryTag::U32));
     execution.add(context, 4, 5, 6);
 }
 

barretenberg/cpp/src/barretenberg/vm2/simulation/memory.hpp

@@ -12,6 +12,8 @@ namespace bb::avm2::simulation {
 struct ValueRefAndTag {
     const MemoryValue& value;
     MemoryTag tag;
+
+    bool operator==(const ValueRefAndTag& other) const { return value == other.value && tag == other.tag; }
 };
 
 using SliceWithTags = std::pair<std::vector<MemoryValue>, std::vector<MemoryTag>>;
@@ -54,4 +56,4 @@ class Memory : public MemoryInterface {
     EventEmitterInterface<MemoryEvent>& events;
 };
 
-} // namespace bb::avm2::simulation
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation/testing/mock_alu.hpp

@@ -14,10 +14,7 @@ class MockAlu : public AluInterface {
     MockAlu();
     ~MockAlu() override;
 
-    MOCK_METHOD(void,
-                add,
-                (ContextInterface&, MemoryAddress a_addr, MemoryAddress b_addr, MemoryAddress dst_addr),
-                (override));
+    MOCK_METHOD(FF, add, (const ValueRefAndTag& a, const ValueRefAndTag& b), (override));
 };
 
-} // namespace bb::avm2::simulation
+} // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/simulation_helper.cpp

@@ -62,7 +62,7 @@ struct FastSettings {
 template <typename S> EventsContainer AvmSimulationHelper::simulate_with_settings()
 {
     typename S::template DefaultEventEmitter<ExecutionEvent> execution_emitter;
-    typename S::template DefaultEventEmitter<AluEvent> alu_emitter;
+    typename S::template DefaultDeduplicatingEventEmitter<AluEvent> alu_emitter;
     typename S::template DefaultEventEmitter<BitwiseEvent> bitwise_emitter;
     typename S::template DefaultEventEmitter<MemoryEvent> memory_emitter;
     typename S::template DefaultEventEmitter<BytecodeRetrievalEvent> bytecode_retrieval_emitter;

barretenberg/cpp/src/barretenberg/vm2/tracegen/alu_trace.cpp

@@ -40,14 +40,12 @@ void AluTraceBuilder::process(const simulation::EventEmitterInterface<simulation
                       { C::alu_op, static_cast<uint8_t>(event.operation) },
                       { C::alu_ia, event.a },
                       { C::alu_ib, event.b },
-                      { C::alu_ic, event.res },
-                      { C::alu_ia_addr, event.a_addr },
-                      { C::alu_ib_addr, event.b_addr },
-                      { C::alu_dst_addr, event.dst_addr },
+                      { C::alu_ic, event.c },
+                      /* TODO: Add tag */
                   } });
 
         row++;
     }
 }
 
-} // namespace bb::avm2::tracegen
+} // namespace bb::avm2::tracegen

barretenberg/cpp/src/barretenberg/vm2/tracegen/alu_trace.test.cpp

@@ -26,7 +26,7 @@ TEST(AluTraceGenTest, TraceGeneration)
 
     builder.process(
         {
-            { .operation = AluOperation::ADD, .a_addr = 0, .b_addr = 1, .dst_addr = 2, .a = 1, .b = 2, .res = 3 },
+            { .operation = AluOperation::ADD, .a = 1, .b = 2, .c = 3, .tag = MemoryTag::U32 /* unused for now */ },
         },
         trace);
 
@@ -35,9 +35,6 @@ TEST(AluTraceGenTest, TraceGeneration)
                     // Only one row.
                     AllOf(ROW_FIELD_EQ(R, alu_op, static_cast<uint8_t>(AluOperation::ADD)),
                           ROW_FIELD_EQ(R, alu_sel_op_add, 1),
-                          ROW_FIELD_EQ(R, alu_ia_addr, 0),
-                          ROW_FIELD_EQ(R, alu_ib_addr, 1),
-                          ROW_FIELD_EQ(R, alu_dst_addr, 2),
                           ROW_FIELD_EQ(R, alu_ia, 1),
                           ROW_FIELD_EQ(R, alu_ib, 2),
                           ROW_FIELD_EQ(R, alu_ic, 3))));

barretenberg/cpp/src/barretenberg/vm2/tracegen/execution_trace.cpp

@@ -19,6 +19,8 @@ constexpr size_t operand_columns = 4;
 
 } // namespace
 
+// TODO: Currently we accept the execution opcode, we need a way to map this to the actual selector for the circuit
+// we should be able to leverage the instruction specification table for this
 void ExecutionTraceBuilder::process(
     const simulation::EventEmitterInterface<simulation::ExecutionEvent>::Container& orig_events, TraceContainer& trace)
 {