Struct instance forward-declaration

[asinghani]

Hello, I've been using sv2v for a few weeks now and found it extremely useful in my work - it has made it much easier to maintain consistency across toolchains even when using the latest SystemVerilog features, however I noticed a change in the latest version that appears to cause invalid verilog code to be generated when structs are used in a certain order. Below is a simple reproducible example:

The following SystemVerilog (specifically note struct2 being declared after it is used):

typedef struct packed {
    logic my_value;
} my_struct_t;

module top (
    output wire led,
    input wire btn,

    input wire clk
);

my_struct_t struct1;

assign struct1.my_value = btn;
wire test = (struct1.my_value == struct2.my_value);
assign led = test;

my_struct_t struct2;

always @(posedge clk) begin
    struct2 <= struct1;
end

endmodule

With versions of sv2v up to commit hash 211e4b0, the following output code is generated, which is valid:

module top (
        led,
        btn,
        clk
);
        output wire led;
        input wire btn;
        input wire clk;
        wire [0:0] struct1;
        assign struct1[0] = btn;
        wire test = struct1[0] == struct2[0];
        assign led = test;
        reg [0:0] struct2;
        always @(posedge clk) struct2 <= struct1;
endmodule

However, at commit hash 85e3d0f, struct2 is simply ignored and the struct notation is inserted into the output verilog code, which then fails to synthesize.

module top (
        led,
        btn,
        clk
);
        output wire led;
        input wire btn;
        input wire clk;
        wire [0:0] struct1;
        assign struct1[0] = btn;
        wire test = struct1[0] == struct2.my_value;
        assign led = test;
        reg [0:0] struct2;
        always @(posedge clk) struct2 <= struct1;
endmodule

The same SystemVerilog code synthesizes properly in yosys when used with the SystemVerilog mode, so this appears to be bug introduced during the refactoring/optimization.

[zachjs]

Thank you for filing such a detailed issue!

The commit you referenced introduced changes requiring items to be declared before they are used for certain conversions to work properly. As I'm sure you guessed, moving the declaration of struct2 before its usage will produce the expected results.

This change in sv2v's behavior provided for much more straightforward handling of generate block scoping, and was based on Section 6.5 of IEEE 1800-2017, which states:

Data shall be declared before they are used, apart from implicit nets (see 6.10).

To my surprise, many commercial tools appear to accept your example. Even more interesting, if I remove the wrapping struct as shown below, most commercial tools reject the input, complaining about the declaration order.

module top (
    output wire led,
    input wire btn,

    input wire clk
);

logic struct1;

assign struct1 = btn;
wire test = (struct1 == struct2);
assign led = test;

logic struct2;

always @(posedge clk) begin
    struct2 <= struct1;
end

endmodule

Perhaps there is some aspect of the specification I am overlooking?

[zachjs]

It's worth noting that the valid output you've given from the older version of sv2v is also rejected for the declaration order issue by 3 of the 4 commercial simulators I attempted to pass it through. I still don't understand why those tools don't complain when the logic is wrapped in a struct, as the relevant semantics should be the same.

I've made a branch called reorder (https://github.com/zachjs/sv2v/tree/reorder) which attempts to automatically fix declaration order issues. Please let me know how that works for you. On that branch, your example now yields:

module top (
    led,
    btn,
    clk
);
    output wire led;
    input wire btn;
    input wire clk;
    wire [0:0] struct1;
    assign struct1[0] = btn;
    reg [0:0] struct2;
    wire test = struct1[0] == struct2[0];
    assign led = test;
    always @(posedge clk) struct2 <= struct1;
endmodule

Notice that the definition of struct2 is automatically hoisted and struct conversion now completes successfully.

The strategy I'm using will not necessarily work for all such issues. I'm not sure if it would make sense to make this behavior default, or to put it behind a flag.

[asinghani]

That does seem to work, thanks for the fix! I'd imagine it would make more sense to put it behind a flag esp if it wouldn't always work, but it would be nice to at least have an error message when the condition is hit and the flag isn't set (as opposed to generating invalid code entirely).

[zachjs]

Adding this as the default behavior will probably be more helpful than not. I've made it so that the overhead associated with this additional work should be relatively low. This change (with a small fix) has been pushed to master and the reorder branch has been deleted.

[zachjs]

I believe this issue has been resolved. If not, please feel free to reopen it.