zachjs / sv2v

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

False incompatible bus size error on output and reg #252

Closed stitchlibar closed 11 months ago

stitchlibar commented 11 months ago

sv2v complains about a simple output/reg coding style sv2v.exe: declarations output [32 - 1:0] data and reg [31:0] data are incompatible due to different packed dimensions

module top(/AUTOARG/ // Outputs data, // Inputs clk, rst, din ); input clk, rst; input [2:0] din; output [32-1:0] data; reg [31:0] data; always @(posedge clk) begin data <= 32'h0; //{32{din[0]}}; end endmodule

zachjs commented 11 months ago

Thanks for reporting this issue! I believe the issue should be fixed in 07caba64a5eb2612bde3dbc05a35150a31e6a170. Note that there remains some pathological cases where non-ANSI style port declarations can still be incorrectly reported as incompatible. Please let me know if you run into further issues!

stitchlibar commented 11 months ago

Thanks. How to get this branch? Will the following git get the version you just made? git clone https://github.com/zachjs/sv2v.git

zachjs commented 11 months ago

Yes, pulling the latest will include the fix mentioned above.

stitchlibar commented 11 months ago

Thanks. It fixes pure digital operation in bus width. But when the bus width has parameter, it still fails. Check below, ouput [MYPAR-1:0] out and reg [7:0] out The latest sv2v complains: declarations output [MYPAR - 1:0] out and reg [7:0] out are incompatible due to different packed dimensions

module top(/AUTOARG/ // Outputs out, // Inputs clk, in ); parameter MYPAR = 8; input clk; input [7:0] in; output [MYPAR-1:0] out; reg [7:0] out; always @(posedge clk) out <= in; endmodule

zachjs commented 11 months ago

Indeed those declarations are compatible, but this usage is bumping up against some limitations of the tool. sv2v doesn't instantiate regular modules and only does a limited amount of constant folding. Thus, it doesn't "know" MYPAR is always set to 8, and hence that the declarations are compatible.

I have some ideas of how to adapt sv2v to address this. To help inform this work, I would like to better understand your use case. It may be most helpful to share your actual design rather than a reduced example so I can get a sense for the real usage.

Though this latest example is valid, it is pathological in that MYPAR can never be set to anything other than its default value. I'd like to better understand your actual use case. Is there a reason someone would want to use:

parameter P = 8;
output [P-1:0] out;
reg [7:0] out;

and not

parameter P = 8;
output [P-1:0] out;
reg [P-1:0] out;

or

output [7:0] out;
reg [7:0] out;

?

To be clear, if P were a localparam, this would seem a bit less unusual to me. sv2v doesn't yet simplify the localparam before combining the declarations in such a scenario, but this could be fixed.

stitchlibar commented 11 months ago

The real case is using define instead of parameter. Can 'define' be simplified? It's actually a not well coded verilog from an existing proven IP.

define P 8 output [P-1:0] out; reg [7:0] out;

stitchlibar commented 11 months ago

Backtick seems a key word in the github system. So it can't display well. Use ' instead 'define P 8 output ['P-1:0] out; reg [7:0] out;

zachjs commented 11 months ago

`define can indeed be simplified. The following is indeed accepted by sv2v after the change I made above:

module top(out);
    `define P 8
    output [`P-1:0] out;
    reg [7:0] out;
endmodule

Is there a similar example where sv2v incorrectly rejects them as incompatible?

stitchlibar commented 11 months ago

Yes, 'define works now. Thanks. I don't think there will be case for parameter or localparam. The issue can be closed.