Addressing review fedback

barretenberg/cpp/pil/vm2/bitwise.pil

@@ -17,7 +17,7 @@ sel * (1 - sel) = 0;
 
 // No relations will be checked if this identity is satisfied.
 #[skippable_if]
-sel = 0;
+sel + last = 0; // They are both boolean so it corresponds to sel == 0 AND last == 0.
 
 pol commit start; // Identifies when we want to capture the output to the main trace.
                   // No need to constrain start to be a boolean, we only need it to select
@@ -101,7 +101,11 @@ sel {op_id, ia_byte, ib_byte, ic_byte}
 in
 precomputed.sel_bitwise {precomputed.bitwise_op_id, precomputed.bitwise_input_a, precomputed.bitwise_input_b, precomputed.bitwise_output};
 
+// TODOs: See two following paragraphs
+
+// ################################################
 // Alternative implementation as potential speed-up
+// ################################################
 //
 // In vm1, we had an approach which requires one extra row per bitwise operation but 
 // had 2 less columns and #[BITW_CTR_DECREMENT] would have degree 0 and the degree 4 relation
@@ -112,4 +116,20 @@ precomputed.sel_bitwise {precomputed.bitwise_op_id, precomputed.bitwise_input_a,
 // Note that sel == 0 on last row of each operation, but the skippable condition
 // remains valid as the last row will be empty with our witness generator.
 //
-// It might be worth to measure the difference among both approaches.
+// It might be worth to measure the difference among both approaches.
+
+
+// ################################################
+// Recycling of bitwise operations of prefixes
+// ################################################
+//
+// Observation: If two inputs are prefixes of other inputs which are already present in the
+//              trace, then we could retrieve the result as a truncated trace of the larger.
+//
+// For instance, re-using example at the top, we consider the U16 and computation over
+// a = 0x5248 
+// b = 0xC684
+// c = 0x4200
+// Then, we should activate the start selector where ctr == 2, and the following rows
+// represent a valid trace for this computation.
+// It is not clear if this would lead to some speed-up in practice.

barretenberg/cpp/src/barretenberg/vm2/common/memory_types.cpp

@@ -20,6 +20,7 @@ uint8_t integral_tag_length(MemoryTag tag)
         throw std::runtime_error("FF is not an integral tag");
     }
 
+    assert(false && "This should not happen");
     return 0; // Should never happen. To please the compiler.
 }
 

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/bitwise.test.cpp

@@ -20,6 +20,7 @@ using tracegen::TestTraceContainer;
 using FF = AvmFlavorSettings::FF;
 using C = Column;
 using bitwise = bb::avm2::bitwise<FF>;
+using testing::SizeIs;
 
 TEST(BitwiseConstrainingTest, EmptyRow)
 {
@@ -55,7 +56,10 @@ TEST(BitwiseConstrainingTest, AndWithTracegen)
         },
         trace);
 
-    check_relation<bitwise>(trace.as_rows());
+    auto rows = trace.as_rows();
+    EXPECT_THAT(rows, SizeIs(33)); // 33 = 1 + 1 + 1 + 2 + 4 + 8 + 16 (extra_shift_row U1 U8 U16 U32 U64 U128)
+
+    check_relation<bitwise>(rows);
 }
 
 // Testing a positive OR operation for each integral type (U1, U8, ... U128)
@@ -83,7 +87,9 @@ TEST(BitwiseConstrainingTest, OrWithTracegen)
         },
         trace);
 
-    check_relation<bitwise>(trace.as_rows());
+    auto rows = trace.as_rows();
+    EXPECT_THAT(rows, SizeIs(33)); // 33 = 1 + 1 + 1 + 2 + 4 + 8 + 16 (extra_shift_row U1 U8 U16 U32 U64 U128)
+    check_relation<bitwise>(rows);
 }
 
 // Testing a positive XOR operation for each integral type (U1, U8, ... U128)
@@ -113,7 +119,9 @@ TEST(BitwiseConstrainingTest, XorWithTracegen)
         },
         trace);
 
-    check_relation<bitwise>(trace.as_rows());
+    auto rows = trace.as_rows();
+    EXPECT_THAT(rows, SizeIs(33)); // 33 = 1 + 1 + 1 + 2 + 4 + 8 + 16 (extra_shift_row U1 U8 U16 U32 U64 U128)
+    check_relation<bitwise>(rows);
 }
 
 TEST(BitwiseConstrainingTest, MixedOperationsWithTracegen)
@@ -132,13 +140,14 @@ TEST(BitwiseConstrainingTest, MixedOperationsWithTracegen)
         },
         trace);
 
-    EXPECT_EQ(trace.get_num_rows(), 13); // 13 = 3 * 1 + 1 * 2 + 2 * 4 (2U1 + 1U8 + 1U16 + 2U32)
-    check_relation<bitwise>(trace.as_rows());
+    auto rows = trace.as_rows();
+    EXPECT_THAT(rows, SizeIs(14)); // 14 = 1 + 3 * 1 + 1 * 2 + 2 * 4 (extra_shift_row + 2U1 + 1U8 + 1U16 + 2U32)
+    check_relation<bitwise>(rows);
 }
 
 TEST(BitwiseConstrainingTest, NegativeWrongInit)
 {
-    TestTraceContainer::RowTraceContainer trace = {
+    TestTraceContainer test_container = TestTraceContainer::from_rows({
         {
             .bitwise_acc_ia = 25,
             .bitwise_acc_ib = 25,
@@ -148,11 +157,12 @@ TEST(BitwiseConstrainingTest, NegativeWrongInit)
             .bitwise_ic_byte = 28,
             .bitwise_last = 1,
         },
-    };
+    });
 
-    EXPECT_THROW_WITH_MESSAGE(check_relation<bitwise>(trace, bitwise::SR_BITW_INIT_A), "BITW_INIT_A");
-    EXPECT_THROW_WITH_MESSAGE(check_relation<bitwise>(trace, bitwise::SR_BITW_INIT_B), "BITW_INIT_B");
-    EXPECT_THROW_WITH_MESSAGE(check_relation<bitwise>(trace, bitwise::SR_BITW_INIT_C), "BITW_INIT_C");
+    auto rows = test_container.as_rows();
+    EXPECT_THROW_WITH_MESSAGE(check_relation<bitwise>(rows, bitwise::SR_BITW_INIT_A), "BITW_INIT_A");
+    EXPECT_THROW_WITH_MESSAGE(check_relation<bitwise>(rows, bitwise::SR_BITW_INIT_B), "BITW_INIT_B");
+    EXPECT_THROW_WITH_MESSAGE(check_relation<bitwise>(rows, bitwise::SR_BITW_INIT_C), "BITW_INIT_C");
 }
 
 TEST(BitwiseConstrainingTest, NegativeTruncateCtr)

barretenberg/cpp/src/barretenberg/vm2/generated/relations/bitwise.hpp

@@ -17,7 +17,7 @@ template <typename FF_> class bitwiseImpl {
     template <typename AllEntities> inline static bool skip(const AllEntities& in)
     {
         const auto& new_term = in;
-        return (new_term.bitwise_sel).is_zero();
+        return ((new_term.bitwise_sel + new_term.bitwise_last)).is_zero();
     }
 
     template <typename ContainerOverSubrelations, typename AllEntities>

barretenberg/cpp/src/barretenberg/vm2/simulation/bitwise.cpp

@@ -7,37 +7,49 @@
 
 namespace bb::avm2::simulation {
 
-void Bitwise::and_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c)
+uint128_t Bitwise::and_op(MemoryTag tag, const uint128_t& a, const uint128_t& b)
 {
+    const uint128_t c = a & b;
+
     events.emit({
         .operation = BitwiseOperation::AND,
         .tag = tag,
         .a = a,
         .b = b,
         .res = c,
     });
+
+    return c;
 }
 
-void Bitwise::or_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c)
+uint128_t Bitwise::or_op(MemoryTag tag, const uint128_t& a, const uint128_t& b)
 {
+    const uint128_t c = a | b;
+
     events.emit({
         .operation = BitwiseOperation::OR,
         .tag = tag,
         .a = a,
         .b = b,
         .res = c,
     });
+
+    return c;
 }
 
-void Bitwise::xor_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c)
+uint128_t Bitwise::xor_op(MemoryTag tag, const uint128_t& a, const uint128_t& b)
 {
+    const uint128_t c = a ^ b;
+
     events.emit({
         .operation = BitwiseOperation::XOR,
         .tag = tag,
         .a = a,
         .b = b,
         .res = c,
     });
+
+    return c;
 }
 
 } // namespace bb::avm2::simulation

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

@@ -10,12 +10,13 @@
 
 namespace bb::avm2::simulation {
 
+// TODO(fcarreiro): think if it makes sense to have memory types like in TS with implicit tag
 class BitwiseInterface {
   public:
     virtual ~BitwiseInterface() = default;
-    virtual void and_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c) = 0;
-    virtual void or_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c) = 0;
-    virtual void xor_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c) = 0;
+    virtual uint128_t and_op(MemoryTag tag, const uint128_t& a, const uint128_t& b) = 0;
+    virtual uint128_t or_op(MemoryTag tag, const uint128_t& a, const uint128_t& b) = 0;
+    virtual uint128_t xor_op(MemoryTag tag, const uint128_t& a, const uint128_t& b) = 0;
 };
 
 class Bitwise : public BitwiseInterface {
@@ -25,9 +26,9 @@ class Bitwise : public BitwiseInterface {
     {}
 
     // Operands are expected to be direct.
-    void and_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c) override;
-    void or_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c) override;
-    void xor_op(MemoryTag tag, uint128_t a, uint128_t b, uint128_t c) override;
+    uint128_t and_op(MemoryTag tag, const uint128_t& a, const uint128_t& b) override;
+    uint128_t or_op(MemoryTag tag, const uint128_t& a, const uint128_t& b) override;
+    uint128_t xor_op(MemoryTag tag, const uint128_t& a, const uint128_t& b) override;
 
   private:
     // TODO: Use deduplicating events

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

@@ -12,9 +12,9 @@ class MockBitwise : public BitwiseInterface {
     MockBitwise();
     ~MockBitwise() override;
 
-    MOCK_METHOD(void, and_op, (MemoryTag tag, uint128_t a, uint128_t b, uint128_t c), (override));
-    MOCK_METHOD(void, or_op, (MemoryTag tag, uint128_t a, uint128_t b, uint128_t c), (override));
-    MOCK_METHOD(void, xor_op, (MemoryTag tag, uint128_t a, uint128_t b, uint128_t c), (override));
+    MOCK_METHOD(uint128_t, and_op, (MemoryTag tag, const uint128_t& a, const uint128_t& b), (override));
+    MOCK_METHOD(uint128_t, or_op, (MemoryTag tag, const uint128_t& a, const uint128_t& b), (override));
+    MOCK_METHOD(uint128_t, xor_op, (MemoryTag tag, const uint128_t& a, const uint128_t& b), (override));
 };
 
 } // namespace bb::avm2::simulation

barretenberg/cpp/src/barretenberg/vm2/tracegen/bitwise_trace.cpp

@@ -11,14 +11,15 @@
 
 namespace bb::avm2::tracegen {
 
-using namespace bb::avm2;
-
 void BitwiseTraceBuilder::process(const simulation::EventEmitterInterface<simulation::BitwiseEvent>::Container& events,
                                   TraceContainer& trace)
 {
     using C = Column;
 
-    uint32_t row = 0;
+    // We activate last selector in the extra pre-pended row (to support shift)
+    trace.set(C::bitwise_last, 0, 1);
+
+    uint32_t row = 1;
     for (const auto& event : events) {
         const auto start_ctr = integral_tag_length(event.tag);
 

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

@@ -35,10 +35,24 @@ TEST(AvmTraceGenBitwiseTest, U1And)
         },
         trace);
 
-    EXPECT_EQ(trace.as_rows().size(), 1);
+    EXPECT_EQ(trace.as_rows().size(), 2);
 
     EXPECT_THAT(trace.as_rows(),
-                ElementsAre(AllOf(ROW_FIELD_EQ(R, bitwise_op_id, static_cast<uint8_t>(BitwiseOperation::AND)),
+                ElementsAre(AllOf(ROW_FIELD_EQ(R, bitwise_op_id, 0),
+                                  ROW_FIELD_EQ(R, bitwise_acc_ia, 0),
+                                  ROW_FIELD_EQ(R, bitwise_acc_ib, 0),
+                                  ROW_FIELD_EQ(R, bitwise_acc_ic, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ia_byte, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ib_byte, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ic_byte, 0),
+                                  ROW_FIELD_EQ(R, bitwise_tag, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ctr, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ctr_inv, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ctr_min_one_inv, 0),
+                                  ROW_FIELD_EQ(R, bitwise_last, 1),
+                                  ROW_FIELD_EQ(R, bitwise_sel, 0),
+                                  ROW_FIELD_EQ(R, bitwise_start, 0)),
+                            AllOf(ROW_FIELD_EQ(R, bitwise_op_id, static_cast<uint8_t>(BitwiseOperation::AND)),
                                   ROW_FIELD_EQ(R, bitwise_acc_ia, 0),
                                   ROW_FIELD_EQ(R, bitwise_acc_ib, 1),
                                   ROW_FIELD_EQ(R, bitwise_acc_ic, 0),
@@ -71,10 +85,24 @@ TEST(AvmTraceGenBitwiseTest, U32And)
         },
         trace);
 
-    EXPECT_EQ(trace.as_rows().size(), 4);
+    EXPECT_EQ(trace.as_rows().size(), 5);
 
     EXPECT_THAT(trace.as_rows(),
-                ElementsAre(AllOf(ROW_FIELD_EQ(R, bitwise_op_id, static_cast<uint8_t>(BitwiseOperation::AND)),
+                ElementsAre(AllOf(ROW_FIELD_EQ(R, bitwise_op_id, 0),
+                                  ROW_FIELD_EQ(R, bitwise_acc_ia, 0),
+                                  ROW_FIELD_EQ(R, bitwise_acc_ib, 0),
+                                  ROW_FIELD_EQ(R, bitwise_acc_ic, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ia_byte, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ib_byte, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ic_byte, 0),
+                                  ROW_FIELD_EQ(R, bitwise_tag, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ctr, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ctr_inv, 0),
+                                  ROW_FIELD_EQ(R, bitwise_ctr_min_one_inv, 0),
+                                  ROW_FIELD_EQ(R, bitwise_last, 1),
+                                  ROW_FIELD_EQ(R, bitwise_sel, 0),
+                                  ROW_FIELD_EQ(R, bitwise_start, 0)),
+                            AllOf(ROW_FIELD_EQ(R, bitwise_op_id, static_cast<uint8_t>(BitwiseOperation::AND)),
                                   ROW_FIELD_EQ(R, bitwise_acc_ia, 0x52488425),
                                   ROW_FIELD_EQ(R, bitwise_acc_ib, 0xC684486C),
                                   ROW_FIELD_EQ(R, bitwise_acc_ic, 0x42000024),

barretenberg/cpp/src/barretenberg/vm2/tracegen/precomputed_trace.cpp

@@ -52,6 +52,8 @@ void PrecomputedTraceBuilder::process_bitwise(TraceContainer& trace)
         case BitwiseOperation::XOR:
             return a ^ b;
         }
+
+        assert(false && "This should not happen");
         return 0; // Should never happen. To please the compiler.
     };