Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error / warning on continous assignment to reg #1369

Open
veripoolbot opened this issue Nov 28, 2018 · 17 comments
Open

Raise error / warning on continous assignment to reg #1369

veripoolbot opened this issue Nov 28, 2018 · 17 comments
Assignees
Labels
area: data-types Issue involves data-types area: lint Issue involves SystemVerilog lint checking effort: hours Expect this issue to require roughly hours of invested effort to resolve

Comments

@veripoolbot
Copy link
Contributor


Author Name: Peter Gerst
Original Redmine Issue: 1369 from https://www.veripool.org

Original Assignee: Wilson Snyder (@wsnyder)


Verilator does not throw error / warning on continuous assignment to a register. See the following example:

content of wire_test.v:

module wire_test (
     input clk,
     input rst,
     input in1,
     input in2,
     input mux,
     output reg out
);

// caught by verilator git version 15af70
// wire internal_reg;
reg internal_reg;
reg out_muxed;

always @(posedge clk) begin
     if (rst) begin
         internal_reg <= 1'b0;
     end else begin
         internal_reg <= in1;
     end
end

assign out_muxed = (mux) ? internal_reg: in2;
assign out = (rst) ? out_muxed : 1'b0;

endmodule

Calling verilator as @verilator --lint-only wire_test.v@ does not complain about the assignment on internal_reg and out registers.

git version of verilator is used (last commit on Nov 26) but the issue is also present in version 4.006:

$ git log -n 1
commit 15af706286212c933595ba8228d2d334fb81e0f7 (HEAD -> master, origin/master, origin/HEAD)
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Mon Nov 26 19:09:08 2018 -0500

     Fix crash due to cygwin bug in getline, #�.

This issue is in connection with forum message https://www.veripool.org/boards/3/topics/1559-Verilator-Verilator-fails-to-warn-error-on-procedural-assignment-to-wire that seems to be corrected: Procedural assignment to wire causes error message to be thrown using git version referenced above.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-11-29T23:12:10Z


I believe the code sent is legal in SystemVerilog, so no warning is appropriate. What complains and does it support SystemVerilog?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Peter Gerst
Original Date: 2018-11-30T06:50:38Z


Xilinx ISE 14.7 syntheser complains about this:

ERROR:HDLCompiler:329 - "wire_test\wire_test.v" Line 23: Target <out_muxed> of concurrent assignment or output port connection should be a net type.
ERROR:HDLCompiler:329 - "wire_test\wire_test.v" Line 24: Target <out> of concurrent assignment or output port connection should be a net type.
ERROR:HDLCompiler:598 - "wire_test\wire_test.v" Line 1: Module <wire_test> ignored due to previous errors.

I need to use this syntheser for Spartan 6 targets. As far as I know it does not support SystemVerilog.
(https://www.xilinx.com/support/documentation/sw_manuals/xilinx14_4/sim.pdf page 11 and 101)

I also forgot to mention that I use verilator with specifying the Verilog standard that ISE supports. For linting verilator is called in the following way:

verilator --lint-only --default-language 1364-2001 -Wall -Wno-PINCONNECTEMPTY wire_test.v

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-12-01T15:25:53Z


Fixed in git towards 4.008.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-12-01T20:17:32Z


In 4.008.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Peter Gerst
Original Date: 2018-12-04T06:46:18Z


Thank you for the fix! verilator now raises error on internal_reg but does not recognize the case of out signal in the example code above.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kris Jeon
Original Date: 2019-08-18T01:09:25Z


In order to raise the warning for port, I've changed like the following:

In 'V3ParseGrammar.cpp'

AstVar* V3ParseGrammar::createVariable(FileLine* fileline, string name,
                                        AstNodeRange* arrayp, AstNode* attrsp) {
     AstNodeDType* dtypep = GRAMMARP->m_varDTypep;
     UINFO(5,"  creVar "<<name<<"  decl="<<GRAMMARP->m_varDecl
           <<"  io="<<GRAMMARP->m_varIO<<"  dt="<<(dtypep?"set":"")<<endl);
     // added lines -->
     if (v3Global.opt.lintOnly() && !fileline->language().systemVerilog()
         && GRAMMARP->m_varDecl == AstVarType::PORT && GRAMMARP->m_varIO == VDirection::OUTPUT && (!dtypep)) {
         GRAMMARP->m_varDecl = AstVarType::WIRE;
     } // <--
...

In 'V3ParseGrammar.cpp'

     virtual void visit(AstNodeVarRef* nodep) {
         // Any variable
         if (nodep->lvalue()
             && !VN_IS(nodep, VarXRef)) {  // Ignore interface variables and similar ugly items
             if (m_inProcAssign && !nodep->varp()->varType().isProcAssignable()) {
                 nodep->v3warn(PROCASSWIRE, "Procedural assignment to wire, perhaps intended var"
                               " (IEEE 2017 6.5): "
                               +nodep->prettyName());
             }
             if (m_inContAssign && !nodep->varp()->varType().isContAssignable()
                 && !nodep->fileline()->language().systemVerilog()) {
                 nodep->v3warn(CONTASSREG, "Continuous assignment to reg, perhaps intended wire"
                               " (IEEE 2005 6.1; Verilog only, legal in SV): "
                               +nodep->prettyName());
             }
             // added lines -->
             if (v3Global.opt.lintOnly() && m_inContAssign && nodep->varp()->varType() == AstVarType::PORT
                 && !nodep->fileline()->language().systemVerilog()) {
                 nodep->v3warn(CONTASSREG, "Continuous assignment to reg, perhaps intended wire"
                               " (IEEE 2005 6.1; Verilog only, legal in SV): "
                               +nodep->prettyName());
             } // <--
         }

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-27T20:40:03Z


I wouldn't have thought your patch would be needed as the unreleased git version of verilator should have fixed this about a month ago. Can you please check it? If there is a problem with those warnings please file a new issue.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kris Jeon
Original Date: 2019-08-28T12:10:07Z


Hi,

It's been modified from 4.016. 4.016 version doesn't warn continuous assignments for the port (out) declared as output reg in wire_test.v.

%Error-CONTASSREG: wire_test.v:34: Continuous assignment to reg, perhaps intended wire (IEEE 2005 6.1; Verilog only, legal in SV): out_muxed
%Error: Exiting due to 1 error(s)
         ... See the manual and http://www.veripool.org/verilator for more assistance.

The modified version warns like the following:

%Error-CONTASSREG: wire_test.v:34: Continuous assignment to reg, perhaps intended wire (IEEE 2005 6.1; Verilog only, legal in SV): out_muxed
%Error-CONTASSREG: wire_test.v:35: Continuous assignment to reg, perhaps intended wire (IEEE 2005 6.1; Verilog only, legal in SV): out
%Error: Exiting due to 2 error(s)
         ... See the manual and http://www.veripool.org/verilator for more assistance.

I missed something ?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-08-28T12:34:45Z


Please use the unreleased git version, it intended to fix this.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kris Jeon
Original Date: 2019-09-03T11:23:51Z


Oh, where can I get the unreleased git version ?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-03T11:55:42Z


See https://www.veripool.org/projects/verilator/wiki/Installing

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Kris Jeon
Original Date: 2019-09-03T12:06:54Z


Is the git version different from tarball version ?I thought that they were the same.
I'll have to use git version. Thank you for information.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-03T12:23:32Z


Git is the change-by-change repo, which is snapshotted for the tarballs.

Anyhow the version released this weekend has the change you want, so use the tarball if you want.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Peter Gerst
Original Date: 2019-11-27T14:07:23Z


Warning about assigning to output reg does not work for me. I'm using tag 4.022 without success.

verilator --lint-only --default-language 1364-2001 -Wall -Wno-PINCONNECTEMPTY wire_test.v

warns only about out_muxed but not about out using the initial example code in this issue. There is no warning after declaration of out_muxed has been corrected from reg to wire.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-07T17:44:07Z


Sorry this has been such a mess, will make another pass. Getting this right is surprisingly difficult...

@veripoolbot veripoolbot added area: data-types Issue involves data-types area: lint Issue involves SystemVerilog lint checking effort: hours Expect this issue to require roughly hours of invested effort to resolve labels Dec 22, 2019
@PeterMonsson
Copy link
Contributor

I think that the remainder of this issue is due to Verilator seeing the varType() of "out" as a port. It appears to me that Verilator doesn't differentiate between a reg output and a wire output.

Specifically this condition in V3Undriven cause the error to not be printed: !nodep->varp()->varType().isContAssignable()

@wsnyder
Copy link
Member

wsnyder commented Sep 24, 2020

That sounds roughly right. If you could please make a pull it would be appreciated, also please check against the verilator_ext_tests as these warnings tend to have false positives until properly tuned. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: data-types Issue involves data-types area: lint Issue involves SystemVerilog lint checking effort: hours Expect this issue to require roughly hours of invested effort to resolve
Projects
None yet
Development

No branches or pull requests

3 participants