zachjs / sv2v

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

Zero-width signals issue in combination with Verilator #163

Closed flaviens closed 1 year ago

flaviens commented 3 years ago

Another issue here :slightly_smiling_face:

Verilator does not appreciate the output of sv2v in the case of zero-width signals. You can generate an example by:

git clone git@github.com:flaviens/zero_width_issue.git
cd zero_width_issue
make

It generates the following errors:

%Error: riscv_decoder.v:358:26: Replication value of 0 is only legal under a concatenation (IEEE 1800-2017 11.4.12.1)
                              : ... In instance riscv_decoder
  358 |   apu_type_o = {WAPUTYPE {1'sb0}};
      |                          ^
%Error: riscv_decoder.v:361:31: Replication value of 0 is only legal under a concatenation (IEEE 1800-2017 11.4.12.1)
                              : ... In instance riscv_decoder
  361 |   apu_flags_src_o = {WAPUTYPE {1'sb0}};
      |                               ^

However, the file before applying sv2v is accepted by Verilator, as can be experimented with make reference.

Thank you! :slightly_smiling_face:

zachjs commented 3 years ago

To generate cleaner output, sv2v assumes that W > 0 in declaration dimensions like [W-1:0] or [0:W-1]. In this case, the default insertion of riscv_decore uses WAPUTYPE = 0, which violates this assumption. That said, I'm inclined to argue that the default instantiation of this module is invalid. When instantiated with valid parameter values, verilator will not raise these errors. What do you think?

flaviens commented 3 years ago

I would tend to totally agree to your opinion 👍 . However, in this case, this module is instantiated with WAPUTYPE=0 in the default PULPissimo design iirc. Although this looks like an arguably poor practice, it seems to exist in significant designs. I think sv2v would greatly profit of being compatible with such imperfect (in terms of good coding practices) designs.

zachjs commented 3 years ago

I've pushed a change to avoid making the RHS the exact width in simple cases like x = '1. While there are related cases (e.g., ternary expressions) which could encounter this issue, I'd like to see how far you can get with this simple adjustment.

flaviens commented 3 years ago

Sounds reasonable and solves the issues for PULPissimo! Thanks! :+1:

flaviens commented 1 year ago

Hi Zach,

I just stumbled into the same kind of issue again, this time it looks like:

input:

module some_module(
    input logic a,
    output logic y
);

  some_submodule sub_i (
      .a ( a ),
      .b ( b ),
      .some_zeros ( '0 )
  );

endmodule

output:

module some_module (
    a,
    y
);
    input wire a;
    output wire y;
    wire b;
    some_submodule sub_i(
        .a(a),
        .b(b),
        .some_zeros(1'sb0) // I'd loved to have '0 here instead
    );
endmodule

Is this someting that you could fix? Alternatively I could do some post-processing, but this may help some other people as well. Thanks! Flavien

zachjs commented 1 year ago

Although your question also deals with unbased unsized literals, I believe it is unrelated to the original issue, unless some_zeros has zero-width, as was the case for, e.g., apu_type_o above. I'm going to assume some_zeros does not have zero width.

Using 1'sb0 in place of '0 in this case is, I believe, functionally equivalent. However, downstream tools may complain when sign-extending port connections. There are two straightforward options.

  1. sv2v only uses 1'sb0 here because it doesn't know the type of some_zeros. If you pass the definition of both some_module and some_submodule in one invocation, sv2v will use that information to generate a constant of the appropriate width. I've provided an example at the end of this comment.
  2. If your downstream tool already supports unbased unsized literals, you can make sv2v pass them through by specifying -E UnbasedUnsized.

Input:

module some_module(
    input logic a,
    output logic y
);
    parameter P = 10;
    some_submodule #(P) sub_i(
        .a(a),
        .b(b),
        .some_zeros('0)
    );
endmodule
module some_submodule #(
    parameter W = 0
) (
    input logic a,
    output logic b,
    input logic [W - 1:0] some_zeros
);
endmodule

Output:

module some_module (
    a,
    y
);
    input wire a;
    output wire y;
    parameter P = 10;
    localparam sv2v_uu_sub_i_W = P;
    localparam [sv2v_uu_sub_i_W - 1:0] sv2v_uu_sub_i_ext_some_zeros_0 = 1'sb0;
    wire b;
    some_submodule #(.W(P)) sub_i(
        .a(a),
        .b(b),
        .some_zeros(sv2v_uu_sub_i_ext_some_zeros_0)
    );
endmodule
module some_submodule (
    a,
    b,
    some_zeros
);
    parameter W = 0;
    input wire a;
    output wire b;
    input wire [W - 1:0] some_zeros;
endmodule
flaviens commented 1 year ago

Hi Zach,

Thank you for your response! It answers perfectly :+1: .