zachjs / sv2v

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

Full-zero inputs reduced to one single bit #182

Closed flaviens closed 2 years ago

flaviens commented 3 years ago

Hi Zach,

I would like to report what seems to be a bug in sv2v that may remain undetected using current open source tools, but which critically affects commercial simulators.

Let's consider the following module:

module main;
  second_module i_second_module (
    .dual_bit_input('0) // expects 2 bits
  );
endmodule

This module is currently transformed by sv2v to

module main;
        second_module i_second_module(.dual_bit_zero_input(1'sb0));
endmodule

Let's additionally assume that second_module takes a 2-bit input (this is not known at sv2v processing time).

Then, sv2v's output is wrong:

This error may have remained unnoticed so far, since Verilator does not seem to support high impedance signals (and probably treats them as zeros).

Thanks!

zachjs commented 3 years ago

Section 10.8 of IEEE 1800-2017 lists "A port connection to an input or output port of a module, interface, or program" as one of many "assignment-like contexts." This implies that port connections should behave like continuous assignments, including in the behavior of width/sign extension.

To my knowledge

Note that Verilator's behavior is not due to a lack of support for high-impedance signals, e.g., 1'sb1 connected to a wider port sign extends rather than padding with zeroes.

Of course, if the module definition is available, sv2v will appropriately size the converted literal. If sv2v does not know the width of the port AND it cannot rely on the downstream tool to perform width extension, it's not clear to me what else it can do here, unless it is to leave the expression unconverted. What do you think?

flaviens commented 3 years ago

Hi Zach,

Thank you for the good explanations! I have read and experimented also on my side, and there does not seem to be a good solution in IEEE 1364-2005 that would be able to replace SystemVerilog's '0 when the width is unknown a priori.

If the goal of sv2v is to provide input to Yosys, then keeping '0 and '1 is a good solution, because Yosys's read_verilog -sv recognizes them. However, this does not comply with the target Verilog standard.

I believe there's unfortunately no perfect decision, it's up to you! :slightly_smiling_face:

TheLoneWolfling commented 3 years ago

I don't suppose you could add an option to --exclude to pass through this? And/or a warning for this case?

(One other possibility is to expand '0 / '1 in such cases to a wide constant, but a) this causes warnings in some cases, and b) it's not obvious what size to expand to, as a single port can be an arbitrary width. (And c) you can't always do so, see also --oversized-numbers.))

zachjs commented 3 years ago

@TheLoneWolfling I've just added such an option to --exclude. Please let me know if that works for you!

TheLoneWolfling commented 3 years ago

That was quick. Thank you!

Seems to work at a quick test.

(FYI, --help and the readme are out of date, but that's minor.)

zachjs commented 3 years ago

FYI, --help and the readme are out of date, but that's minor.

Thank you for pointing this out! I've fixed it.

zachjs commented 2 years ago

It seems the current behavior is pretty reasonable given the constraints, especially with addition of --exclude UnbasedUnsized. As such, I am closing this issue. If either of you encounter any problems, please feel free to reopen or file a new issue!