Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-12-19T23:17:45Z

Structures can reference structures and arrays, so this is a lot harder than enums. If you'd like to work more on a patch an I can give suggestions as to how to proceed.

@wsnyder I'd like to work on a patch. where should I start?

Great, we'd appreciate your contribution.

So,

1. make a test including making it mark the appropriate publics (probably include packed struct, unpacked struct, union). There needs to be two ways tested, one as metacomments and the other as .vlt file scripted commands.
2. improve verilog.l/verilog.y to parse the new metacomment/command and set a public attribute on the AstNodeUOrStructDType, which is base class of struct and union.
3. Fortunately, all of the information about the struct is already carried through to the final stages, so nothing might be needed in the middle stages.
4. Emit the appropriate code, V3EmitCSyms.cpp does similar stuff now. Ideally we would create an intermediate representation and print that, but given the current code doesn't do that I suppose we can skip that extra step.

@wsnyder Thanks. I'll take a stab at it.

Hi, it seems that verilator only support unpacked struct or union by this commit. If no one is working on this, I'd like to make a patch for support on packed struct or union.

Here is my proposed function of exporting packed struct / union. A packed struct / union can contain another packed struct / union, enum type or basic integral. Exported packed struct / union in C++ should be consistent with packed struct / union in SV at bit level, which is useful for top struct port. If verilator can't achieve this, issue a warning like ALIGNPACKED.

An example extracted from my SV design.

  typedef enum logic [1:0] {
    M0  = 2'b00,
    M1 = 2'b01,
    M2 = 2'b10,
    M3 = 2'b11
  } w_e;

  typedef struct packed {
    logic[5:0]   o;
    w_e   w;
  } vtype_t;

  typedef struct packed {
    logic[10:0]          a;
    logic[10:0]          b;
    vtype_t                c;
  } context_t; 

Which can be exported as

enum w_e: uint8_t { M0 = 0U, M1 = 1U, M2 = 2U, M3 = 3U };
struct vtype_t {
    w_e w : 2;
    uint8_t o : 6;
};
struct context_t {
    vtype_t c;
    uint32_t b : 10;
    uint16_t a : 10;
};

Great! Some details:

The struct should always be either 8, 16, 32, 64, or N*32 bits to correspond to the Verilator data types (using CData SData IData QData or EData as appropriate). So add a _pad member to make that be true. With that I don't think you ever will need a warning.

Note the bit order in structs is not defined by the C standard. You will need configure to run a test to determine it, and set a define appropriately, then generate code with both options (Unless C++ has a standard define already.)

For example

struct context_t {
#ifdef BIG_BIT_ENDIAN // need better name - is there a standard?
    IData _pad : 12;
    IData b : 10;
    IData a : 10;
#else
    IData a : 10;
    IData b : 10;
    IData _pad : 12;
#endif
};

When generating these, have an assert that the total size matches the right type, e.g. it sums up to the 32 and asserts that 32 == sizeof(IData)*8 /bits-per-byte/;

Thank you for the informative reply! It reminds me of endianness and VlWide class. I would revise expected goal of exporting packed struct / union as making sure that a simple memcpy could transfer data between exported packed sturct / union and int types or VlWide class, these internal data representation in verilator.

But I'm afraid that a new warning is still needed due to the fact that not all structs can be exported with satisfying above requirement. For example

typedef struct packed {
  logic [62:0] a;
  logic [3:0]  b;
} m_t;

field a, b can't be placed adjacently at C level. Or another possible solution is to split field b into 2 separate field in the exported struct.

Before considering endianness. Please allow me to confirm my understanding about how verilator maps SV bits into C bits: the low bit of SV data is mapped into corresponding LSB of int types or VlWide class, and this mapping is independent of endianness.

If my above understanding is correct. The width of EData also has an impact on exported struct / union. For example, consider following SV struct

typedef struct packed {
  logic [32:0] a;
  logic [30:0]  b;
  logic [32:0] c;
  logic [30:0]  d;
} m_t;  // 128 bits

Currently, width of EData is 32 bits. Therefore, m_t will be stored as VlWide<4> in verilator. And m_t can be exported at little-endian machine as

struct m_t {
    QData d: 31;
    QData c: 33; 
    QData b: 31;
    QData a: 33;
};

But restricted by width of EData, there is no proper way to export m_t at big-endian machine. Or, again we can spilt field b and d as

struct m_t {
    EData c_SPLIT_LOW: 1;
    EData d: 31;
    EData c_SPLIT_HIGH: 32;
    EData a_SPLIT_LOW: 1;
    EData b: 31;
    EData a_SPLIT_HIGH: 32;
};

Which implementation is preferred?

how verilator maps SV bits into C bits: the low bit of SV data is mapped into corresponding LSB of int types or VlWide class, and this mapping is independent of endianness.

Yes,

In Verilog, the last one in a struct is least significant and includes bit zero.

In Verilator, bit 0 is of course bit 0 in the given data type that stores the structure, e.g. for WData it would be word number 0 bit number 0 (that is can read it with == 1 & (data[0] >> 0)).

What is not specified by C/C++ is the ordering of bitfields in C structs.

But restricted by width of EData, there is no proper way to export m_t at big-endian machine. Or, again we can spilt field b and d as

struct m_t {
   EData c_SPLIT_LOW: 1;
   EData d: 31;
   EData c_SPLIT_HIGH: 32;
   EData a_SPLIT_LOW: 1;
   EData b: 31;
   EData a_SPLIT_HIGH: 32;
};

Yes, I think we need to split as you indicate when a structure is > 64 bits. A field may need more than 2 pieces (have a test for that). As to naming, use __ to avoid conflicts with user signals, maybe call these word numbers?.

 EData c__w0: 1;
 EData d: 31;
 EData c__w1: 32;
 EData a__w0: 1;
 EData b: 31;
 EData a__w1: 32;

What is not specified by C/C++ is the ordering of bitfields in C structs.

Yes, but I think (by some tests) the implementation is the same for gcc and clang. It's some part of ABI, and many programs dealing with network protocol rely on it. So I consider the layout rule used by gcc and clang as a de-facto standard.

I forgot to say another corner case caused by embedded array or struct. For example

typedef logic [7:0] udata8_t;
typedef struct packed {
  logic [3:0] a;
  udata8_t [2:0] b;
} m_t;

typedef struct packed {
  m_t b;
  logic [3:0] a;
} n_t;

For no support of bitfield array or bitfield struct in C, we can export above struct only when the field is aligned naturally. So m_t can be exported for zero bit offset of packed array b. Exporting n_t is more difficult instead. I have some doubts whether it's friendly to user If we split array or flatten embedded struct recursively.

So I tend to issue a warning for non-aligned array or struct and don't export the packed struct / union?

I'm not aware of any network libraries of significance that doesn't code to allow either style. See e.g. https://github.com/torvalds/linux/blob/master/include/uapi/linux/ip.h __LITTLE_ENDIAN_BITFIELD/__BIG_ENDIAN_BITFIELD. FWIW also gcc/clang support either endinness based on how built, and network order is NOT standard x86/Arm little endian order.

Anyhow, I don't want to rely on assuming gcc/clang ordering.

Because C++ don't specify the storage layout of bitfield. If we want to fill the bitfield of struct and use a simple memcpy to transfer data into VlWide class, there must be some assumption about how compiler will organize these bitfields in memory. Just like the example you list.

struct iphdr {
#if defined(__LITTLE_ENDIAN_BITFIELD)
	__u8	ihl:4,
		version:4;
#elif defined (__BIG_ENDIAN_BITFIELD)
	__u8	version:4,
  		ihl:4;
#else
#error	"Please fix <asm/byteorder.h>"
#endif
};

At least linux will assume field ihl and version will be packed into one byte. In addition, bitfield will be packed from LSB in little-endian machine and from MSB in big-endian machine. With these two assumptions, ihl field will always be in the first four LSB of the first byte, and version field in the last four LSB, independent of endianness of machine.

Or, instead of exporting pure struct and relying on unportable bitfield storage layout, we could export a class with the same storage layout as internal representation of verilator and some manipulation member function. Users call these functions to initialize SV struct member. Within each function, we use portable bitmask to set corresponding bits, just like how verilator extracts bits from SV struct.

Hi @ckf104 you might want to take a look at this tool I wrote to export SV structs and enums to C++. By any means I want you to stop pushing for this verilator feature (I would like to see this implemented in verilator), but you might get some ideas on the API (or not).

Hi @Sustrak. Thank you for your points and I didn't know this tool before.

I have tried the slang-reflect and it's indeed helpful in some degree. And it will be more useful to me if it could export a version without dependency on systemc.

by any means I want you to stop pushing for this verilator feature (I would like to see this implemented in verilator)

I'm sorry I don't get it. Do you have any plan to implement this exporting feature recently? For the implementation is similar to my proposed second implementation above.

Based on the example maybe making accessor functions is a better route, then we can also hide padding.

Hi @Sustrak. Thank you for your points and I didn't know this tool before.

I have tried the slang-reflect and it's indeed helpful in some degree. And it will be more useful to me if it could export a version without dependency on systemc.

You can use --no-sc to avoid having the SystemC dependency. The only drawback is that you can not have members bigger than 64 bits

Here are my proposed API currently.

• AstBasicDType and AstEnumDType embedded in the packed struct / union will be extended as CData, SData, IData, Qdata or VlWide class based on their width in the exported struct / union.
• AstNodeUOrStructDType embedded in the packed struct / union will remain the same in the exported struct / union.
• AstPackArrayDType embedded in the packed struct / union will be transferred as array in C with corresponding transferred element type.
• Exported struct has two member functions, one accepts internal verilator struct / union representation and initialize struct fields, the other returns internal verilator struct / union representation by current field values.

For example:

  typedef enum logic [1:0] {
    M0  = 2'b00,
    M1 = 2'b01,
    M2 = 2'b10,
    M3 = 2'b11
  } w_e;

  typedef struct packed {
    logic[5:0]   o;
    w_e   w;
  } vtype_t;

typedef logic [1:0]  data2_t;

  typedef struct packed {
    logic[10:0]  a;
    data2_t  [4:0]  b;
    vtype_t  c;
  } context_t; 

The context_t struct will be exported as

struct context_t {
SData a;
CData [4:0] b;
vtype_t c;

// `context_t` is 29 bits in SV, represented as IData in verilator
IData get() {
   // return a verilator representation by value of `a`, `b`, `c`  and bitmask
}
void set(IData v){
   // set `a`, `b`, `c` by argument v
}
}

Users are expected to initialize exported struct or get value by accessing struct field directly.

If this API seems reasonable, I can make a patch these days.

Hi, @Sustrak. It seems to me that slang-reflect don't support the --no-sc option, I have opened a issue for this.

That plan seems reasonable. I'd suggest before coding the V3Emit*.cpp to make this, make a small example verilog test, Verilate it, make the corresponding struct API c++ manually, and compile that up and see how it connects up and works hooking up the test. Then you can make a first pull request for discussion, then proceed on automating creating what you made manually.

Hi, @wsnyder. I feel a bit confused on implementation of exporting unpacked struct / union in verilator. Consider following example

package TEST_TYPES;

   typedef struct {
      logic a;
   } b_struct_t ;
endpackage // TEST_TYPES

module t;
   TEST_TYPES::b_struct_t b;
   initial begin
      b.a = 1'b1;
      $display("b.a=%b", b.a);
   end

endmodule

Test by lastest verilator, even though I don't set /*verilator public*/, verilator will output header file for package TEST_TYPES and exporting struct b_struct_t. Instead, defining a enum type don't avoid package node being removed from AST.

The root cause comes from following code in V3dead.cpp

    bool shouldDeleteTypedef(AstTypedef* typedefp) {
        if (auto* const structp = VN_CAST(typedefp->subDTypep(), NodeUOrStructDType)) {
            if (structp->user1() && !structp->packed()) return false;
        }
        return m_elimCells && !typedefp->attrPublic();
    }

AstTypedef won't be deleted if corresponding struct / union has non zero reference count.

So, is it desired to output package header file and export unpacked struct as long as it's defined in a package?

Yes, starting at structures/unions, if they are public, you will need to recurse down all data types and look for enums/typedefs and mark them public. Then V3Dead might need to know some new kind of publics (e.g. typedefs) and increment user1() which will make them persist.

But for this test case, even though no data type marked as public, the latest verilator will still export struct b_struct_t, is it a bug in verilator?

package TEST_TYPES;

   typedef struct {
      logic a;
   } b_struct_t ;
endpackage // TEST_TYPES

module t;
   TEST_TYPES::b_struct_t b;
   initial begin
      b.a = 1'b1;
      $display("b.a=%b", b.a);
   end

endmodule

I will fix it by the way if it's a bug.

CHANGELOG:

Oh, I have misunderstood the implementation of unpacked struct in verilator. The unpacked struct will be represented by C++ struct anyway. Sorry for this noise

Thanks for your work on this.