diff --git a/Changes b/Changes index c6bb9622b..e2b4d0440 100644 --- a/Changes +++ b/Changes @@ -5,6 +5,8 @@ indicates the contributor was also the author of the fix; Thanks! * Verilator 3.847 devel +*** Add ALWCOMBORDER warning. [KC Buckenmaier] + *** Add --pins-sc-uint and --pins-sc-biguint, bug638. [Alex Hornung] **** Fix module resolution with __, bug631. [Jason McMullan] diff --git a/bin/verilator b/bin/verilator index c87cb2a4c..2616245b6 100755 --- a/bin/verilator +++ b/bin/verilator @@ -1068,10 +1068,10 @@ Disable the specified warning message. =item -Wno-lint Disable all lint related warning messages, and all style warnings. This is -equivalent to "-Wno-CASEINCOMPLETE -Wno-CASEOVERLAP -Wno-CASEX --Wno-CASEWITHX -Wno-CMPCONST -Wno-ENDLABEL -Wno-IMPLICIT -Wno-LITENDIAN --Wno-PINMISSING -Wno-SYNCASYNCNET -Wno-UNDRIVEN -Wno-UNSIGNED -Wno-UNUSED --Wno-WIDTH" plus the list shown for Wno-style. +equivalent to "-Wno-ALWCOMBORDER -Wno-CASEINCOMPLETE -Wno-CASEOVERLAP +-Wno-CASEX -Wno-CASEWITHX -Wno-CMPCONST -Wno-ENDLABEL -Wno-IMPLICIT +-Wno-LITENDIAN -Wno-PINMISSING -Wno-SYNCASYNCNET -Wno-UNDRIVEN +-Wno-UNSIGNED -Wno-UNUSED -Wno-WIDTH" plus the list shown for Wno-style. It is strongly recommended you cleanup your code rather than using this option, it is only intended to be use when running test-cases of code @@ -1100,9 +1100,10 @@ Enables the specified warning message. Enable all lint related warning messages (note by default they are already enabled), but do not affect style messages. This is equivalent to -"-Wwarn-CASEINCOMPLETE -Wwarn-CASEOVERLAP -Wwarn-CASEX -Wwarn-CASEWITHX --Wwarn-CMPCONST -Wwarn-ENDLABEL -Wwarn-IMPLICIT -Wwarn-LITENDIAN --Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED -Wwarn-WIDTH". +"-Wwarn-ALWCOMBORDER -Wwarn-CASEINCOMPLETE -Wwarn-CASEOVERLAP -Wwarn-CASEX +-Wwarn-CASEWITHX -Wwarn-CMPCONST -Wwarn-ENDLABEL -Wwarn-IMPLICIT +-Wwarn-LITENDIAN -Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED +-Wwarn-WIDTH". =item -Wwarn-style @@ -2714,6 +2715,20 @@ List of all warnings: =over 4 +=item ALWCOMBORDER + +Warns that an always_comb block has a variable which is set after it is +used. This may cause simulation-synthesis mismatches, as not all +commercial simulators allow this ordering. + + always_comb begin + a = b; + b = 1; + end + +Ignoring this warning will only suppress the lint check, it will simulate +correctly. + =item ASSIGNIN Error that an assignment is being made to an input signal. This is almost diff --git a/src/V3Assert.cpp b/src/V3Assert.cpp index 0c8eff7e5..f42e24c6b 100644 --- a/src/V3Assert.cpp +++ b/src/V3Assert.cpp @@ -137,6 +137,7 @@ private: if (nodep->castPslAssert()) ifp->branchPred(AstBranchPred::BP_UNLIKELY); // AstNode* newp = new AstAlways (nodep->fileline(), + VAlwaysKwd::ALWAYS, sentreep, bodysp); // Install it diff --git a/src/V3Ast.h b/src/V3Ast.h index f27a7725b..7d58a3f61 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -467,6 +467,30 @@ public: //###################################################################### +class VAlwaysKwd { +public: + enum en { + ALWAYS, + ALWAYS_FF, + ALWAYS_LATCH, + ALWAYS_COMB + }; + enum en m_e; + inline VAlwaysKwd () : m_e(ALWAYS) {} + inline VAlwaysKwd (en _e) : m_e(_e) {} + explicit inline VAlwaysKwd (int _e) : m_e(static_cast(_e)) {} + operator en () const { return m_e; } + const char* ascii() const { + static const char* names[] = { + "always","always_ff","always_latch","always_comb"}; + return names[m_e]; } + }; + inline bool operator== (VAlwaysKwd lhs, VAlwaysKwd rhs) { return (lhs.m_e == rhs.m_e); } + inline bool operator== (VAlwaysKwd lhs, VAlwaysKwd::en rhs) { return (lhs.m_e == rhs); } + inline bool operator== (VAlwaysKwd::en lhs, VAlwaysKwd rhs) { return (lhs == rhs.m_e); } + +//###################################################################### + class AstCaseType { public: enum en { diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 3f70b3332..cfa015b7d 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -677,6 +677,11 @@ void AstNode::dump(ostream& str) { if (name()!="") str<<" "<AstNode::dump(str); + if (keyword() != VAlwaysKwd::ALWAYS) str<<" ["<AstNode::dump(str); str<<" [start:"<castSenTree(); } // op1 = Sensitivity list AstNode* bodysp() const { return op2p()->castNode(); } // op2 = Statements to evaluate void addStmtp(AstNode* nodep) { addOp2p(nodep); } + VAlwaysKwd keyword() const { return m_keyword; } // Special accessors bool isJustOneBodyStmt() const { return bodysp() && !bodysp()->nextp(); } }; @@ -1702,7 +1706,7 @@ struct AstAssignW : public AstNodeAssign { AstAlways* convertToAlways() { AstNode* lhs1p = lhsp()->unlinkFrBack(); AstNode* rhs1p = rhsp()->unlinkFrBack(); - AstAlways* newp = new AstAlways (fileline(), NULL, + AstAlways* newp = new AstAlways (fileline(), VAlwaysKwd::ALWAYS, NULL, new AstAssign (fileline(), lhs1p, rhs1p)); replaceWith(newp); // User expected to then deleteTree(); return newp; diff --git a/src/V3ClkGater.cpp b/src/V3ClkGater.cpp index 1556549c5..ea0819ee1 100644 --- a/src/V3ClkGater.cpp +++ b/src/V3ClkGater.cpp @@ -684,6 +684,7 @@ class GaterVisitor : public GaterBaseVisitor { AstNode* bodyp = nodep->bodysp()->cloneTree(true); AstAlways* alwp = new AstAlways(nodep->fileline(), + nodep->keyword(), sensesp, bodyp); diff --git a/src/V3Error.h b/src/V3Error.h index 692f9881f..1f9030c46 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -57,6 +57,7 @@ public: // Warning codes: EC_FIRST_WARN, // Just a code so the program knows where to start warnings // + ALWCOMBORDER, // Always_comb with unordered statements ASSIGNDLY, // Assignment delays ASSIGNIN, // Assigning to input BLKANDNBLK, // Blocked and non-blocking assignments to same variable @@ -116,7 +117,7 @@ public: "BLKLOOPINIT", "DETECTARRAY", "MULTITOP", "TASKNSVAR", // Warnings " EC_FIRST_WARN", - "ASSIGNDLY", "ASSIGNIN", + "ALWCOMBORDER", "ASSIGNDLY", "ASSIGNIN", "BLKANDNBLK", "BLKSEQ", "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CDCRSTLOGIC", "CMPCONST", "COMBDLY", "DEFPARAM", "DECLFILENAME", @@ -146,7 +147,8 @@ public: bool mentionManual() const { return ( m_e==EC_FATALSRC || pretendError() ); } // Warnings that are lint only - bool lintError() const { return ( m_e==CASEINCOMPLETE || m_e==CASEOVERLAP + bool lintError() const { return ( m_e==ALWCOMBORDER + || m_e==CASEINCOMPLETE || m_e==CASEOVERLAP || m_e==CASEWITHX || m_e==CASEX || m_e==CMPCONST || m_e==ENDLABEL diff --git a/src/V3Split.cpp b/src/V3Split.cpp index d7d5f5bbc..e0b27a6d7 100644 --- a/src/V3Split.cpp +++ b/src/V3Split.cpp @@ -416,7 +416,7 @@ private: } if (splitAlwaysp) { ++m_statSplits; - AstAlways* alwaysp = new AstAlways(newListp->fileline(), NULL, NULL); + AstAlways* alwaysp = new AstAlways(newListp->fileline(), VAlwaysKwd::ALWAYS, NULL, NULL); addAfterp->addNextHere(alwaysp); addAfterp=alwaysp; alwaysp->addStmtp(newListp); } else { diff --git a/src/V3Undriven.cpp b/src/V3Undriven.cpp index d119f7600..0d1d0282a 100644 --- a/src/V3Undriven.cpp +++ b/src/V3Undriven.cpp @@ -133,6 +133,17 @@ public: } } } + bool isUsedNotDrivenBit (int bit, int width) const { + for (int i=0; i UndrivenVar* for usage var, 0=not set yet AstUser1InUse m_inuser1; + AstUser2InUse m_inuser2; // STATE - vector m_entryps; // Nodes to delete when we are finished + vector m_entryps[3]; // Nodes to delete when we are finished bool m_markBoth; // Mark as driven+used AstNodeFTask* m_taskp; // Current task + AstAlways* m_alwaysp; // Current always // METHODS static int debug() { @@ -226,31 +239,45 @@ private: return level; } - UndrivenVarEntry* getEntryp(AstVar* nodep) { - if (!nodep->user1p()) { + UndrivenVarEntry* getEntryp(AstVar* nodep, int which_user) { + if (!(which_user==1 ? nodep->user1p() : nodep->user2p())) { UndrivenVarEntry* entryp = new UndrivenVarEntry (nodep); - m_entryps.push_back(entryp); - nodep->user1p(entryp); + //UINFO(9," Associate u="<name()<user1p(entryp); + else if (which_user==2) nodep->user2p(entryp); + else nodep->v3fatalSrc("Bad case"); return entryp; } else { - UndrivenVarEntry* entryp = (UndrivenVarEntry*)(nodep->user1p()); + UndrivenVarEntry* entryp = (UndrivenVarEntry*)(which_user==1 ? nodep->user1p() : nodep->user2p()); return entryp; } } + void warnAlwCombOrder(AstVarRef* nodep) { + AstVar* varp = nodep->varp(); + if (!varp->isParam() && !varp->isGenVar() && !varp->isUsedLoopIdx() + && !varp->fileline()->warnIsOff(V3ErrorCode::ALWCOMBORDER)) { // Warn only once per variable + nodep->v3warn(ALWCOMBORDER, "Always_comb variable driven after use: "<prettyName()); + varp->fileline()->modifyWarnOff(V3ErrorCode::ALWCOMBORDER, true); // Complain just once for any usage + } + } + // VISITORS virtual void visit(AstVar* nodep, AstNUser*) { - UndrivenVarEntry* entryp = getEntryp (nodep); - if (nodep->isInput() - || nodep->isSigPublic() || nodep->isSigUserRWPublic() - || (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) { - entryp->drivenWhole(); - } - if (nodep->isOutput() - || nodep->isSigPublic() || nodep->isSigUserRWPublic() - || nodep->isSigUserRdPublic() - || (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) { - entryp->usedWhole(); + for (int usr=1; usr<(m_alwaysp?3:2); ++usr) { + UndrivenVarEntry* entryp = getEntryp (nodep, usr); + if (nodep->isInput() + || nodep->isSigPublic() || nodep->isSigUserRWPublic() + || (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) { + entryp->drivenWhole(); + } + if (nodep->isOutput() + || nodep->isSigPublic() || nodep->isSigUserRWPublic() + || nodep->isSigUserRdPublic() + || (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) { + entryp->usedWhole(); + } } // Discover variables used in bit definitions, etc nodep->iterateChildren(*this); @@ -263,10 +290,19 @@ private: AstVarRef* varrefp = nodep->fromp()->castVarRef(); AstConst* constp = nodep->lsbp()->castConst(); if (varrefp && constp && !constp->num().isFourState()) { - UndrivenVarEntry* entryp = getEntryp (varrefp->varp()); - int lsb = constp->toUInt(); - if (m_markBoth || varrefp->lvalue()) entryp->drivenBit(lsb, nodep->width()); - if (m_markBoth || !varrefp->lvalue()) entryp->usedBit(lsb, nodep->width()); + for (int usr=1; usr<(m_alwaysp?3:2); ++usr) { + UndrivenVarEntry* entryp = getEntryp (varrefp->varp(), usr); + int lsb = constp->toUInt(); + if (m_markBoth || varrefp->lvalue()) { + // Don't warn if already driven earlier as "a=0; if(a) a=1;" is fine. + if (usr==2 && m_alwaysp && entryp->isUsedNotDrivenBit(lsb, nodep->width())) { + UINFO(9," Select. Entryp="<<(void*)entryp<drivenBit(lsb, nodep->width()); + } + if (m_markBoth || !varrefp->lvalue()) entryp->usedBit(lsb, nodep->width()); + } } else { // else other varrefs handled as unknown mess in AstVarRef nodep->iterateChildren(*this); @@ -274,10 +310,18 @@ private: } virtual void visit(AstVarRef* nodep, AstNUser*) { // Any variable - UndrivenVarEntry* entryp = getEntryp (nodep->varp()); - bool fdrv = nodep->lvalue() && nodep->varp()->attrFileDescr(); // FD's are also being read from - if (m_markBoth || nodep->lvalue()) entryp->drivenWhole(); - if (m_markBoth || !nodep->lvalue() || fdrv) entryp->usedWhole(); + for (int usr=1; usr<(m_alwaysp?3:2); ++usr) { + UndrivenVarEntry* entryp = getEntryp (nodep->varp(), usr); + bool fdrv = nodep->lvalue() && nodep->varp()->attrFileDescr(); // FD's are also being read from + if (m_markBoth || nodep->lvalue()) { + if (usr==2 && m_alwaysp && entryp->isUsedNotDrivenAny()) { + UINFO(9," Full bus. Entryp="<<(void*)entryp<drivenWhole(); + } + if (m_markBoth || !nodep->lvalue() || fdrv) entryp->usedWhole(); + } } // Don't know what black boxed calls do, assume in+out @@ -288,6 +332,19 @@ private: m_markBoth = prevMark; } + virtual void visit(AstAlways* nodep, AstNUser*) { + AstAlways* prevAlwp = m_alwaysp; + { + AstNode::user2ClearTree(); + if (nodep->keyword() == VAlwaysKwd::ALWAYS_COMB) UINFO(9," "<keyword() == VAlwaysKwd::ALWAYS_COMB) m_alwaysp = nodep; + else m_alwaysp = NULL; + nodep->iterateChildren(*this); + if (nodep->keyword() == VAlwaysKwd::ALWAYS_COMB) UINFO(9," Done "<accept(*this); } virtual ~UndrivenVisitor() { - for (vector::iterator it = m_entryps.begin(); it != m_entryps.end(); ++it) { + for (vector::iterator it = m_entryps[1].begin(); it != m_entryps[1].end(); ++it) { (*it)->reportViolations(); - delete (*it); + } + for (int usr=1; usr<3; ++usr) { + for (vector::iterator it = m_entryps[usr].begin(); it != m_entryps[usr].end(); ++it) { + delete (*it); + } } } }; diff --git a/src/verilog.l b/src/verilog.l index 7fe5a643f..04d296bda 100644 --- a/src/verilog.l +++ b/src/verilog.l @@ -403,9 +403,9 @@ word [a-zA-Z0-9_]+ "$warning" { FL; return yD_WARNING; } /* SV2005 Keywords */ "$unit" { FL; return yD_UNIT; } /* Yes, a keyword, not task */ - "always_comb" { FL; return yALWAYS; } - "always_ff" { FL; return yALWAYS; } - "always_latch" { FL; return yALWAYS; } + "always_comb" { FL; return yALWAYS_COMB; } + "always_ff" { FL; return yALWAYS_FF; } + "always_latch" { FL; return yALWAYS_LATCH; } "bind" { FL; return yBIND; } "bit" { FL; return yBIT; } "break" { FL; return yBREAK; } diff --git a/src/verilog.y b/src/verilog.y index de3745e24..25b29e525 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -273,6 +273,9 @@ class AstSenTree; // Double underscores "yX__Y" means token X followed by Y, // and "yX__ETC" means X folled by everything but Y(s). %token yALWAYS "always" +%token yALWAYS_FF "always_ff" +%token yALWAYS_COMB "always_comb" +%token yALWAYS_LATCH "always_latch" %token yAND "and" %token yASSERT "assert" %token yASSIGN "assign" @@ -1549,7 +1552,10 @@ module_common_item: // ==IEEE: module_common_item | final_construct { $$ = $1; } // // IEEE: always_construct // // Verilator only - event_control attached to always - | yALWAYS event_controlE stmtBlock { $$ = new AstAlways($1,$2,$3); } + | yALWAYS event_controlE stmtBlock { $$ = new AstAlways($1,VAlwaysKwd::ALWAYS, $2,$3); } + | yALWAYS_FF event_controlE stmtBlock { $$ = new AstAlways($1,VAlwaysKwd::ALWAYS_FF, $2,$3); } + | yALWAYS_COMB event_controlE stmtBlock { $$ = new AstAlways($1,VAlwaysKwd::ALWAYS_COMB, $2,$3); } + | yALWAYS_LATCH event_controlE stmtBlock { $$ = new AstAlways($1,VAlwaysKwd::ALWAYS_LATCH, $2,$3); } | loop_generate_construct { $$ = $1; } | conditional_generate_construct { $$ = $1; } // // Verilator only diff --git a/test_regress/t/t_lint_always_comb_bad.pl b/test_regress/t/t_lint_always_comb_bad.pl new file mode 100755 index 000000000..6361176b7 --- /dev/null +++ b/test_regress/t/t_lint_always_comb_bad.pl @@ -0,0 +1,22 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 by Wilson Snyder. This program is free software; you can +# redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. + +compile ( + verilator_flags2 => ["--lint-only"], + verilator_make_gcc => 0, + make_top_shell => 0, + make_main => 0, + fails => 1, + expect=> +'%Warning-ALWCOMBORDER: t/t_lint_always_comb_bad.v:\d+: Always_comb variable driven after use: mid +.*%Error: Exiting due to.*', + ); + +ok(1); +1; diff --git a/test_regress/t/t_lint_always_comb_bad.v b/test_regress/t/t_lint_always_comb_bad.v new file mode 100644 index 000000000..6287ddb44 --- /dev/null +++ b/test_regress/t/t_lint_always_comb_bad.v @@ -0,0 +1,48 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2013 by Wilson Snyder. + +module t (/*AUTOARG*/ + // Outputs + mid, o3, + // Inputs + clk, i3 + ); + input clk; + output logic mid; + input i3; + output logic o3; + + wire [15:0] temp1; + wire [15:0] temp1_d1r; + + logic setbefore; + always_comb begin + setbefore = 1'b1; + if (setbefore) setbefore = 1'b0; // fine + end + + always_comb begin + if (mid) + temp1 = 'h0; + else + temp1 = (temp1_d1r - 'h1); + mid = (temp1_d1r == 'h0); // BAD + end + + always_comb begin + o3 = 'h0; + case (i3) + 1'b1: begin + o3 = i3; + end + default: ; + endcase + end + + always_ff @ (posedge clk) begin + temp1_d1r <= temp1; + end + +endmodule