skywater-pdk
skywater-pdk copied to clipboard
google
Net type in verilog models not explicitly declared
In https://github.com/google/skywater-pdk/issues/181 @tangxifan reported two issues. The include file issue was fixed but the other one was not.
Actual Behavior
Modelsim complains about the following errors:
** ** at /skywater-pdk/libraries/sky130_fd_sc_ms/latest/cells/inv/sky130_fd_sc_ms__inv.behavioral.v(39): (vlog-2892) Net type of 'Y' was not explicitly declared.
@mithro I have tried the updated netlists again. Modelsim still complains the same errors. I have tried some tweaks in the Verilog netlist and it fixed the errors. For example, I changed the line 30 at netlist to
`default_nettype wire
Beyond the
*.behavioral.vnetlists, other netlists also require such changes. For example, the sku130_fd_sc_hd__inv_2.vI would suggest to change like this for all the Verilog netlists. Indeed, I see the `default_nettype wire is defined at the end of these netlists, but my understanding is that Modelsim does not accept the late definition.
Originally posted by @tangxifan in https://github.com/google/skywater-pdk/issues/181#issuecomment-720016422
FYI, this `default_nettype statement is not required and should be just removed. What created the bug was setting it to "none", but there is no need to setting it to "wire" either. This is usually what is done in traditional libraries. I am able to compile on Modelsim when removing this statement.
Verilog has an implicit default net type of wire. This allows one to declare nets implicitly without declaring a type. This becomes dangerous when multi-bit wires are implicitly declared, because the multi-bit wire will be implicitly truncated to a single-bit wire.
Using default_nettype none is a common safety measure to ensure all types are explicitly declared. I think it is reasonable to keep it. The real problem here is exactly what the ModelSim error complains about - the net is not explicitly declared:
module sky130_fd_sc_hd__inv (
Y,
A
);
// Module ports
output Y;
input A;
// Module supplies
supply1 VPWR;
supply0 VGND;
supply1 VPB ;
supply0 VNB ;
// Local signals
wire not0_out_Y;
// Name Output Other arguments
not not0 (not0_out_Y, A );
buf buf0 (Y , not0_out_Y );
endmodule
Y and A have declared directions, but not declared types. The better solution, IMO, is to declare the types.
// Module ports
output wire Y;
input wire A;
or
// Module ports
output Y;
input A;
wire Y;
wire A;
Sure, it's possible to just remote the default_nettype statements, but there is likely to be certain cells which require declaring the type anyways (e.g. FFs). One could argue that cell libraries (should) never use multi-bit wires, so the point is moot. I still think that using default_nettype none is a safer solution, though.
I would tend to disagree here, as I think we should follow what is commonly done in standard cell development. The default_nettype none is never used, and neither the inputs and outputs declared as wire or reg. Usually, all the ports (inputs or outputs) are implicitly defined as wires, even for FF cells, so there is no need to declare their type.
Just because it is typical does not mean it is safe. On one side I see "atypical but safer" and on the other side I see "typical but less safe". Is there any compelling reason to use implicit types (e.g. tool compatibility, correctness, etc.)?
Also it's not the ports that need to be declared in FFs, but the internal wires.
I firmly agree with Edouard. Implicit wires are used everywhere in verilog by everyone. Like everything else in any programming languages, it is just one of the rules you need to remember. There are a vast number of ways to produce verilog that is syntactically correct but functionally dead; failing to explicitly declare wire arrays is just one of them. Also: There are multi-bit wires used in SkyWater standard cells. Only a handful, but see, e.g., muxb16to1 in the HDLL library.
I don't necessarily disagree with the change as much as I disagree with the reasoning.
C++ code is littered with off by 1 errors and unsafe memory accesses. It's something that's "accepted" from the language but yet it causes 70% of security vulnerabilities. It is well known that languages like C++ allow you to shoot yourself in the foot, and as a result there has been a ton of work on linters and sanitizers to try to detect code which is allowed by the language specification but has a high propensity to be incorrect.
Why is Verilog any different? There are tons of ways to shoot yourself in the foot with Verilog. default_nettype none is a safety feature exercised in order to prevent bugs caused by implicit wires. Why would we not exercise a safety feature if it exists?
I can understand the reasoning of "we don't need this safety feature", assumingly because the potential issues are restricted to a small space, but I can't get behind the reasoning of "this is how everyone does it" or "the language has many other problems so this one isn't worth checking".
Implicit wires are used everywhere in verilog by everyone
Not me 😉
Agreeing with Austin on the subject of implicit wires. This is a bad practice and has been forbidden on my team for 20+ years.
M
On Fri, Nov 6, 2020 at 1:39 PM Austin Rovinski [email protected] wrote:
I don't necessarily disagree with the change as much as I disagree with the reasoning.
C++ code is littered with off by 1 errors and unsafe memory accesses. It's something that's "accepted" from the language but yet it causes 70% of security vulnerabilities https://www.zdnet.com/article/microsoft-70-percent-of-all-security-bugs-are-memory-safety-issues/. It is well known that languages like C++ allow you to shoot yourself in the foot, and as a result there has been a ton of work on linters and sanitizers to try to detect code which is allowed by the language specification but has a high propensity to be incorrect.
Why is Verilog any different? There are tons of ways to shoot yourself in the foot with Verilog. default_nettype none is a safety feature exercised in order to prevent bugs caused by implicit wires. Why would we not exercise a safety feature if it exists?
I can understand the reasoning of "we don't need this safety feature", assumingly because the potential issues are restricted to a small space, but I can't get behind the reasoning of "this is how everyone does it" or "the language has many other problems so this one isn't worth checking".
Implicit wires are used everywhere in verilog by everyone
Not me 😉
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/skywater-pdk/issues/198#issuecomment-723311736, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEFG5AHI7ACP66NUV7R6LH3SORUIZANCNFSM4TGXY7TQ .
There should not be any implicit wires in the skywater-pdk, any that exist are an error. This is why the file does;
`default_nettype none
FYI, we have an outstanding feature request for a style lint rule on implicit declarations at https://github.com/google/verible/issues/217.
@mithro : I was not aware either that `default_nettype none was supported by iverilog, or that all of the verilog files in the PDK had been corrected for implicit wires. That probably explains why I was seeing errors generated when simulating the I/Os where there were implicit wires. In that case, all is good.
I am struggling to avoid/ fix this issue, when trying to use Mentor Modelsim. Since default_nettype none is set, but the port type declarations are still missing, it produces said errors. (I am wondering why this is not an issue, when simulating with iverlog)
The port declaration of the functional and power models of the cells are written in the Verilog-1995 style, which allows implicit wire port types. I do not quite see why multi-bit wires could be implicitly truncated to a single-bit wire as mentioned by @rovinski. A quick test in Modelsim did not show such problems.
The verilog functional models explicitly say "Autogenerated, do not modify directly". So I guess it does not make sense to add a wire declaration for each port inside the cell libraries or set default_nettype wire by "hand"?
This would be very good to get fixed. I have now added FuseSoC support to run the caravel regression tests that use this library, but it only works on icarus. All other simulators complain about this. Can we just do a mass search-and-replace to add wire types? Or regenerate the verilog from some other source?
If we could get the PDK libraries to explicitly declare wires, then this discussion would be resolved.
For example:
change output Y;
to output wire Y;
Better yet, just migrate to the Verilog 2001 convention of fully qualified port names.
