Improve field list calculation code

include/bm/bm_sim/calculations.h

@@ -226,7 +226,16 @@ class RawCalculation
 };
 
 
-struct BufBuilder {
+class BufBuilder {
+ public:
+  void push_back_field(header_id_t header, int field_offset);
+  void push_back_constant(const ByteContainer &v, size_t nbits);
+  void push_back_header(header_id_t header);
+  void append_payload();
+
+  void operator()(const Packet &pkt, ByteContainer *buf) const;
+
+ private:
   struct field_t {
     header_id_t header;
     int field_offset;
@@ -241,94 +250,10 @@ struct BufBuilder {
     header_id_t header;
   };
 
+  struct Deparse;  // defined in calculations.cpp
+
   std::vector<boost::variant<field_t, constant_t, header_t> > entries{};
   bool with_payload{false};
-
-  void push_back_field(header_id_t header, int field_offset) {
-    field_t f = {header, field_offset};
-    entries.emplace_back(f);
-  }
-
-  void push_back_constant(const ByteContainer &v, size_t nbits) {
-    // TODO(antonin): general case
-    assert(nbits % 8 == 0);
-    constant_t c = {v, nbits};
-    entries.emplace_back(c);
-  }
-
-  void push_back_header(header_id_t header) {
-    header_t h = {header};
-    entries.emplace_back(h);
-  }
-
-  void append_payload() {
-    with_payload = true;
-  }
-
-  struct Deparse : public boost::static_visitor<> {
-    Deparse(const PHV &phv, ByteContainer *buf)
-      : phv(phv), buf(buf) { }
-
-    char *extend(int more_bits) {
-      int nbits_ = nbits + more_bits;
-      buf->resize((nbits_ + 7) / 8, '\x00');
-      char *ptr = buf->data() + (nbits / 8);
-      nbits = nbits_;
-      // needed ?
-      // if (new_bytes > 0) buf->back() = '\x00';
-      return ptr;
-    }
-
-    int get_offset() const {
-      return nbits % 8;
-    }
-
-    void operator()(const field_t &f) {
-      const Header &header = phv.get_header(f.header);
-      if (!header.is_valid()) return;
-      const Field &field = header.get_field(f.field_offset);
-      // taken from headers.cpp::deparse
-      const int offset = get_offset();
-      field.deparse(extend(field.get_nbits()), offset);
-    }
-
-    void operator()(const constant_t &c) {
-      assert(get_offset() == 0);
-      std::copy(c.v.begin(), c.v.end(), extend(c.nbits));
-    }
-
-    void operator()(const header_t &h) {
-      assert(get_offset() == 0);
-      const Header &header = phv.get_header(h.header);
-      if (header.is_valid()) {
-        header.deparse(extend(header.get_nbytes_packet() * 8));
-      }
-    }
-
-    Deparse(const Deparse &other) = delete;
-    Deparse &operator=(const Deparse &other) = delete;
-
-    Deparse(Deparse &&other) = delete;
-    Deparse &operator=(Deparse &&other) = delete;
-
-    const PHV &phv;
-    ByteContainer *buf;
-    int nbits{0};
-  };
-
-  void operator()(const Packet &pkt, ByteContainer *buf) const {
-    buf->clear();
-    const PHV *phv = pkt.get_phv();
-    Deparse visitor(*phv, buf);
-    std::for_each(entries.begin(), entries.end(),
-                  boost::apply_visitor(visitor));
-    if (with_payload) {
-      size_t curr = buf->size();
-      size_t psize = pkt.get_data_size();
-      buf->resize(curr + psize);
-      std::copy(pkt.data(), pkt.data() + psize, buf->begin() + curr);
-    }
-  }
 };
 
 

src/bm_sim/calculations.cpp

@@ -29,9 +29,98 @@
 
 #include "xxhash.h"
 #include "crc_tables.h"
+#include "extract.h"
 
 namespace bm {
 
+void
+BufBuilder::push_back_field(header_id_t header, int field_offset) {
+  field_t f = {header, field_offset};
+  entries.emplace_back(f);
+}
+
+void
+BufBuilder::push_back_constant(const ByteContainer &v, size_t nbits) {
+  constant_t c = {v, nbits};
+  entries.emplace_back(c);
+}
+
+void
+BufBuilder::push_back_header(header_id_t header) {
+  header_t h = {header};
+  entries.emplace_back(h);
+}
+
+void
+BufBuilder::append_payload() {
+  with_payload = true;
+}
+
+struct BufBuilder::Deparse : public boost::static_visitor<> {
+  Deparse(const PHV &phv, ByteContainer *buf, int init_offset = 0)
+      : phv(phv), buf(buf), nbits(init_offset) { }
+
+  char *extend(int more_bits) {
+    int nbits_ = nbits + more_bits;
+    buf->resize((nbits_ + 7) / 8, '\x00');
+    char *ptr = buf->data() + (nbits / 8);
+    nbits = nbits_;
+    return ptr;
+  }
+
+  int get_offset() const {
+    return nbits % 8;
+  }
+
+  void operator()(const field_t &f) {
+    const Header &header = phv.get_header(f.header);
+    if (!header.is_valid()) return;
+    const Field &field = header.get_field(f.field_offset);
+    // get_offset needs to be called before extend!
+    const auto offset = get_offset();
+    field.deparse(extend(field.get_nbits()), offset);
+  }
+
+  void operator()(const constant_t &c) {
+    // get_offset needs to be called before extend!
+    const auto offset = get_offset();
+    extract::generic_deparse(&c.v[0], c.nbits, extend(c.nbits), offset);
+  }
+
+  void operator()(const header_t &h) {
+    assert(get_offset() == 0);
+    const Header &header = phv.get_header(h.header);
+    if (header.is_valid()) {
+      header.deparse(extend(header.get_nbytes_packet() * 8));
+    }
+  }
+
+  Deparse(const Deparse &other) = delete;
+  Deparse &operator=(const Deparse &other) = delete;
+
+  Deparse(Deparse &&other) = delete;
+  Deparse &operator=(Deparse &&other) = delete;
+
+  const PHV &phv;
+  ByteContainer *buf;
+  int nbits{0};
+};
+
+void
+BufBuilder::operator()(const Packet &pkt, ByteContainer *buf) const {
+  buf->clear();
+  const PHV *phv = pkt.get_phv();
+  Deparse visitor(*phv, buf);
+  std::for_each(entries.begin(), entries.end(),
+                boost::apply_visitor(visitor));
+  if (with_payload) {
+    size_t curr = buf->size();
+    size_t psize = pkt.get_data_size();
+    buf->resize(curr + psize);
+    std::copy(pkt.data(), pkt.data() + psize, buf->begin() + curr);
+  }
+}
+
 namespace hash {
 
 uint64_t xxh64(const char *buffer, size_t s) {

src/bm_sim/extract.h

@@ -21,12 +21,14 @@
 #ifndef BM_SIM_EXTRACT_H_
 #define BM_SIM_EXTRACT_H_
 
+#include <algorithm>  // for std::fill, std::copy
+
 namespace bm {
 
 namespace extract {
 
-static void generic_extract(const char *data, int bit_offset,
-                            int bitwidth, char *dst) {
+static inline void generic_extract(const char *data, int bit_offset,
+                                   int bitwidth, char *dst) {
   int nbytes = (bitwidth + 7) / 8;
 
   if (bit_offset == 0 && bitwidth % 8 == 0) {
@@ -39,7 +41,7 @@ static void generic_extract(const char *data, int bit_offset,
 
   // necessary to ensure correct behavior when shifting right (no sign
   // extension)
-  unsigned char *udata = (unsigned char *) data;
+  auto udata = reinterpret_cast<const unsigned char *>(data);
 
   int offset = bit_offset - dst_offset;
   if (offset == 0) {
@@ -64,6 +66,57 @@ static void generic_extract(const char *data, int bit_offset,
   }
 }
 
+static inline void generic_deparse(const char *data, int bitwidth,
+                                   char *dst, int hdr_offset) {
+  int nbytes = (bitwidth + 7) / 8;
+
+  if (hdr_offset == 0 && bitwidth % 8 == 0) {
+    memcpy(dst, data, nbytes);
+    return;
+  }
+
+  int field_offset = (nbytes << 3) - bitwidth;
+  int hdr_bytes = (hdr_offset + bitwidth + 7) / 8;
+
+  int i;
+
+  // necessary to ensure correct behavior when shifting right (no sign
+  // extension)
+  auto udata = reinterpret_cast<const unsigned char *>(data);
+
+  // zero out bits we are going to write in dst[0]
+  dst[0] &= (~(0xFF >> hdr_offset));
+
+  int offset = field_offset - hdr_offset;
+  if (offset == 0) {
+    std::copy(data + 1, data + hdr_bytes, dst + 1);
+    dst[0] |= udata[0];
+  } else if (offset > 0) {  // shift left
+    // don't know if this is very efficient, we memset the remaining bytes to 0
+    // so we can use |= and preserve what was originally in dst[0]
+    std::fill(&dst[1], &dst[hdr_bytes], 0);
+    for (i = 0; i < hdr_bytes - 1; i++) {
+      dst[i] |= (udata[i] << offset) | (udata[i + 1] >> (8 - offset));
+    }
+    dst[i] |= udata[i] << offset;
+  } else {  // shift right
+    offset = -offset;
+    dst[0] |= (udata[0] >> offset);
+    if (nbytes == 1) {
+      // dst[1] is always valid, otherwise we would not need to shift the field
+      // to the right
+      dst[1] = udata[0] << (8 - offset);
+      return;
+    }
+    for (i = 1; i < hdr_bytes - 1; i++) {
+      dst[i] = (udata[i - 1] << (8 - offset)) | (udata[i] >> offset);
+    }
+    int tail_offset = (hdr_bytes << 3) - (hdr_offset + bitwidth);
+    dst[i] &= ((1 << tail_offset) - 1);
+    dst[i] |= (udata[i - 1] << (8 - offset));
+  }
+}
+
 }  // namespace extract
 
 }  // namespace bm

src/bm_sim/fields.cpp

@@ -20,8 +20,6 @@
 
 #include <bm/bm_sim/fields.h>
 
-#include <algorithm>
-
 #include "extract.h"
 
 namespace bm {
@@ -43,52 +41,7 @@ int Field::extract_VL(const char *data, int hdr_offset, int computed_nbits) {
 }
 
 int Field::deparse(char *data, int hdr_offset) const {
-  if (hdr_offset == 0 && nbits % 8 == 0) {
-    std::copy(bytes.begin(), bytes.end(), data);
-    return nbits;
-  }
-
-  int field_offset = (nbytes << 3) - nbits;
-  int hdr_bytes = (hdr_offset + nbits + 7) / 8;
-
-  int i;
-
-  // necessary to ensure correct behavior when shifting right (no sign
-  // extension)
-  unsigned char *ubytes = (unsigned char *) bytes.data();
-
-  // zero out bits we are going to write in data[0]
-  data[0] &= (~(0xFF >> hdr_offset));
-
-  int offset = field_offset - hdr_offset;
-  if (offset == 0) {
-    std::copy(bytes.begin() + 1, bytes.begin() + hdr_bytes, data + 1);
-    data[0] |= ubytes[0];
-  } else if (offset > 0) {  // shift left
-    // don't know if this is very efficient, we memset the remaining bytes to 0
-    // so we can use |= and preserve what was originally in data[0]
-    std::fill(&data[1], &data[hdr_bytes], 0);
-    for (i = 0; i < hdr_bytes - 1; i++) {
-      data[i] |= (ubytes[i] << offset) | (ubytes[i + 1] >> (8 - offset));
-    }
-    data[i] |= ubytes[i] << offset;
-  } else {  // shift right
-    offset = -offset;
-    data[0] |= (ubytes[0] >> offset);
-    if (nbytes == 1) {
-      // data[1] is always valid, otherwise we would not need to shift the field
-      // to the right
-      data[1] = ubytes[0] << (8 - offset);
-      return nbits;
-    }
-    for (i = 1; i < hdr_bytes - 1; i++) {
-      data[i] = (ubytes[i - 1] << (8 - offset)) | (ubytes[i] >> offset);
-    }
-    int tail_offset = (hdr_bytes << 3) - (hdr_offset + nbits);
-    data[i] &= ((1 << tail_offset) - 1);
-    data[i] |= (ubytes[i - 1] << (8 - offset));
-  }
-
+  extract::generic_deparse(&bytes[0], nbits, data, hdr_offset);
   return nbits;
 }
 

tests/test_calculations.cpp

@@ -188,6 +188,33 @@ TEST_F(CalculationTest, WithConstant) {
   ASSERT_EQ(expected, actual);
 }
 
+TEST_F(CalculationTest, WithConstantNonAligned) {
+  BufBuilder builder;
+
+  // 0x9fd = 1001 1111 1101
+  ByteContainer constant("0x9fd");
+
+  builder.push_back_constant(ByteContainer("0x00"), 4);
+  builder.push_back_field(testHeader1, 0);  // f16
+  builder.push_back_constant(constant, 12);
+
+  Calculation calc(builder, "identity");
+
+  unsigned char pkt_buf[2 * header_size];
+
+  for (size_t i = 0; i < sizeof(pkt_buf); i++) {
+    pkt_buf[i] = 0xff;
+  }
+
+  Packet pkt = get_pkt((const char *) pkt_buf, sizeof(pkt_buf));
+  parser.parse(&pkt);
+
+  auto expected = static_cast<uint64_t>(0x0ffff9fd);
+  auto actual = calc.output(pkt);
+
+  ASSERT_EQ(expected, actual);
+}
+
 TEST_F(CalculationTest, WithPayload) {
   BufBuilder builder;