zachjs / sv2v

SystemVerilog to Verilog conversion
BSD 3-Clause "New" or "Revised" License
497 stars 50 forks source link

Different types in generate branches results in error #265

Open mole99 opened 7 months ago

mole99 commented 7 months ago

Using two different types in two different generate branches for the same signal results in the following error:

sv2v: field 'field1' not found in struct packed {
    logic field2;
}, in expression out.field1, within scope test_3A9E3.genblk1 (use -v to get approximate source location)
CallStack (from HasCallStack):
  error, called at src/Convert/Scoper.hs:377:22 in main:Convert.Scoper

Proprietary simulators I have tried seem to ignore the other inactive branch.

This feature is used in the MPU of the cv32e40x.

Here is a minimal example to reproduce the error:


package my_pkg;
    typedef struct packed {
        logic             field1;
    } type1_t;

    typedef struct packed {
        logic            field2;
    } type2_t;
endpackage

module test import my_pkg::*;
#(
    parameter bit  BRANCH  = 1,
    parameter type MY_TYPE = type1_t
)
(
    output MY_TYPE out
);
    generate
        if (BRANCH) begin
            assign out.field1 = '0;
        end else begin
            assign out.field2 = '0;
        end
    endgenerate
endmodule

module top import my_pkg::*; #() ();

    test
    #(  
        .BRANCH  (1),
        .MY_TYPE (type1_t) 
    )
    test_inst1
    (
        .out ()
    );

    test
    #(  
        .BRANCH  (0),
        .MY_TYPE (type2_t) 
    )
    test_inst2
    (
        .out ()
    );

endmodule
zachjs commented 6 months ago

In the discussion following https://github.com/zachjs/sv2v/issues/155#issuecomment-876814907, I wondered whether sv2v should need to be able to determine that a particular code path is unreachable to short-circuit code that is invalid or cannot be represented downstream. Based on #226 and https://github.com/openhwgroup/cv32e40x/commit/3438fdb052d3c23b30c1363d69cec5caa188abf3, it doesn't seem like cv32e40x required this behavior until earlier this year.

It might be possible to support this pattern by detecting and replacing the invalid branch with a $fatal or similar. However, I didn't design the struct conversion with this in mind, so adding support would likely take some time. I'm interested in your thoughts on this.

mole99 commented 6 months ago

I'm afraid I don't have many thoughts on this topic. For now, I just worked around this issue by duplicating the MPU and enabling one of the paths in each module.

Before, I did not even know it was possible to change types via parameters in SystemVerilog ^^

But it seems that to use type parameters one needs support for this feature, right? Because depending on which type is selected, it has to be handled differently. This could also be handled by the preprocessor (`ifdef/`endif), but is not practical.

The solution you proposed seems to work well for this use case, and I can't think of a scenario where the contents of the invalid branch are needed.

Thanks for taking a look at this! Please know there is no rush to support this right now, since I found a workaround for my use case.