New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for /*verilator public*/ on SV structs too? #860
Comments
Original Redmine Comment 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,
|
@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 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
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 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 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 If my above understanding is correct. The width of 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 struct m_t {
QData d: 31;
QData c: 33;
QData b: 31;
QData a: 33;
}; But restricted by width of 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? |
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 What is not specified by C/C++ is the ordering of bitfields in C structs.
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?.
|
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 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 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 |
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 @Sustrak. Thank you for your points and I didn't know this tool before. I have tried the
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. |
You can use |
Here are my proposed API currently.
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 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. |
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 The root cause comes from following code in 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();
}
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 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
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. |
Author Name: Jonathon Donaldson
Original Redmine Issue: 860 from https://www.veripool.org
Original Date: 2014-12-11
The recently added feature of being able to use /verilator public/ on SV enums is priceless and works very very well. I no longer have to duplicate (i.e. recreate) any enums in my C++ code that already exist in my SV code. This saves loads of tedious copy & paste work when creating the testbench!
I'm now wondering if the same capability can easily be added for SV structs? The reason I ask is because I also have a lot of SV structs in my designs that are made up of these enums and so I end up having to duplicate the SV structs in my C++ code.
Let's assume I have the following SV package that I verilate:
With the current version of verilator the "/verilator public/" on the struct is ignored. So this results in having to do the following in the C++ testbench code:
But if /verilator public/ worked on SV structs then all I would have to do is this:
With the existing support for /verilator public/ on SV enums is support for SV structs something that can be easily added? Or are SV structs a whole different problem altogether that would require a significant amount of work?
I attached an example in test_regress format.
The text was updated successfully, but these errors were encountered: