zachjs / sv2v

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

Struct parameters in module definition #246

Closed lpawelcz closed 12 months ago

lpawelcz commented 1 year ago

Hi @zachjs, I tried to use sv2v to convert VeeR EL2 core to Verilog but I came across this behavior where sv2v does not convert correctly module parameters which are included from header files and are defined as struct.

You can reproduce the issue with the following example.

directory structure:

.
├── el2_lib.sv
└── inc
    ├── el2_param.vh
    └── el2_pdef.vh

inc/el2_pdef.vh:

typedef struct packed {
    bit [8:0]    BTB_ADDR_HI;
    bit [8:0]    BTB_BTAG_SIZE;
} el2_param_t;

inc/el2_param.vh:

parameter el2_param_t pt = '{
    BTB_ADDR_HI      : 9'h005,
    BTB_BTAG_SIZE    : 9'h009
}

el2_lib.sv:

module el2_btb_tag_hash #(
    `include "el2_param.vh"
) (
    input logic [pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+1] pc,
    output logic [pt.BTB_BTAG_SIZE-1:0] hash
);

    assign hash = {(
        pc[pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE+1] ^
        pc[pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+1] ^
        pc[pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+1]
    )};
endmodule

Here is the result of converting el2_lib.sv with sv2v -I ./inc -w adjacent el2_lib.sv to el2_lib.v:

module el2_btb_tag_hash (
    pc,
    hash
);
    parameter el2_param_t pt = '{
        BTB_ADDR_HI: 9'h005,
        BTB_BTAG_SIZE: 9'h009
    };
    input wire [((pt.BTB_ADDR_HI + pt.BTB_BTAG_SIZE) + pt.BTB_BTAG_SIZE) + pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI + 1] pc;
    output wire [pt.BTB_BTAG_SIZE - 1:0] hash;
    assign hash = {(pc[((pt.BTB_ADDR_HI + pt.BTB_BTAG_SIZE) + pt.BTB_BTAG_SIZE) + pt.BTB_BTAG_SIZE:((pt.BTB_ADDR_HI + pt.BTB_BTAG_SIZE) + pt.BTB_BTAG_SIZE) + 1] ^ pc[(pt.BTB_ADDR_HI + pt.BTB_BTAG_SIZE) + pt.BTB_BTAG_SIZE:(pt.BTB_ADDR_HI + pt.BTB_BTAG_SIZE) + 1]) ^ pc[pt.BTB_ADDR_HI + pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI + 1]};
endmodule

For more context here is the syntax error from yosys:

el2_lib.v:5: ERROR: syntax error, unexpected TOK_ID, expecting ',' or '=' or ';'

I believe parameter pt should be unfolded into separate parameters: BTB_ADDR_HI and BTB_BTAG_SIZE and the references in the module should also take this into account.

Another thing worth noting about this issue is that it is not strictly related to parameter being included inside the module definition because we can modify el2_lib.sv and manually include the parameter like so:

module el2_btb_tag_hash #(
    parameter el2_param_t pt = '{
        BTB_ADDR_HI      : 9'h005,
        BTB_BTAG_SIZE    : 9'h009
    }
) (
    input logic [pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+1] pc,
    output logic [pt.BTB_BTAG_SIZE-1:0] hash
);

    assign hash = {(
        pc[pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE+1] ^
        pc[pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE+1] ^
        pc[pt.BTB_ADDR_HI+pt.BTB_BTAG_SIZE:pt.BTB_ADDR_HI+1]
    )};
endmodule

and we would still get exactly the same result in converted verilog.

zachjs commented 1 year ago

Thank you for filing a detailed issue! In your example, it doesn't look like anything includes el2_pdef.vh. With the include added el2_lib.sv, I get:

module el2_btb_tag_hash (
    pc,
    hash
);
    parameter [17:0] pt = 18'h00a09;
    input wire [((pt[17-:9] + pt[8-:9]) + pt[8-:9]) + pt[8-:9]:pt[17-:9] + 1] pc;
    output wire [pt[8-:9] - 1:0] hash;
    assign hash = {(pc[((pt[17-:9] + pt[8-:9]) + pt[8-:9]) + pt[8-:9]:((pt[17-:9] + pt[8-:9]) + pt[8-:9]) + 1] ^ pc[(pt[17-:9] + pt[8-:9]) + pt[8-:9]:(pt[17-:9] + pt[8-:9]) + 1]) ^ pc[pt[17-:9] + pt[8-:9]:pt[17-:9] + 1]};
endmodule
lpawelcz commented 1 year ago

Ohhh, you are right! Thank you very much for pointing that out. I experimented a little bit with that and it looks like we can also get the same result with changing the sv2v command by adding the header to FILES:

sv2v -I ./inc -w el2_lib.v el2_lib.sv inc/el2_pdef.vh 

This way I don't have to modify the sources but on the other hand I loose the ability to use -w adjacent because sv2v would exit with: Refusing to write adjacent to "inc/el2_pdef.vh" because that path does not end in ".sv"

It turns out that my initial problem was just incorrect sv2v usage.

zachjs commented 1 year ago

You may also be interested in the write directory feature I just pushed. It's discussed at the end of #218.

lpawelcz commented 1 year ago

That looks awesome, I'll surely test that out. Thanks a lot!

zachjs commented 12 months ago

@lpawelcz Is there anything more to work on for this issue?

lpawelcz commented 12 months ago

Oh sorry for the delay, I think we can close this issue as the problem was on my side. Thank you for your help, I really appreciate that. I also started using this new write directory feature and I must say it is super helpful :)