Add ALWCOMBORDER warning.

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]

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
+equivalent to "-Wno-ALWCOMBORDER -Wno-CASEINCOMPLETE -Wno-CASEOVERLAP
--Wno-CASEWITHX -Wno-CMPCONST -Wno-ENDLABEL -Wno-IMPLICIT -Wno-LITENDIAN
+-Wno-CASEX -Wno-CASEWITHX -Wno-CMPCONST -Wno-ENDLABEL -Wno-IMPLICIT
--Wno-PINMISSING -Wno-SYNCASYNCNET -Wno-UNDRIVEN -Wno-UNSIGNED -Wno-UNUSED
+-Wno-LITENDIAN -Wno-PINMISSING -Wno-SYNCASYNCNET -Wno-UNDRIVEN
--Wno-WIDTH" plus the list shown for Wno-style.
+-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-ALWCOMBORDER -Wwarn-CASEINCOMPLETE -Wwarn-CASEOVERLAP -Wwarn-CASEX
--Wwarn-CMPCONST -Wwarn-ENDLABEL -Wwarn-IMPLICIT -Wwarn-LITENDIAN
+-Wwarn-CASEWITHX -Wwarn-CMPCONST -Wwarn-ENDLABEL -Wwarn-IMPLICIT
--Wwarn-PINMISSING -Wwarn-REALCVT -Wwarn-UNSIGNED -Wwarn-WIDTH".
+-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

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

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<en>(_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 {

src/V3AstNodes.cpp

@@ -677,6 +677,11 @@ void AstNode::dump(ostream& str) {
     if (name()!="") str<<"  "<<AstNode::quoteName(name());
 }
 
+void AstAlways::dump(ostream& str) {
+    this->AstNode::dump(str);
+    if (keyword() != VAlwaysKwd::ALWAYS) str<<" ["<<keyword().ascii()<<"]";
+}
+
 void AstArraySel::dump(ostream& str) {
     this->AstNode::dump(str);
     str<<" [start:"<<start()<<"] [length:"<<length()<<"]";

src/V3AstNodes.h

@@ -1623,15 +1623,19 @@ public:
 };
 
 struct AstAlways : public AstNode {
-    AstAlways(FileLine* fl, AstSenTree* sensesp, AstNode* bodysp)
+    VAlwaysKwd m_keyword;
-	: AstNode(fl) {
+public:
+    AstAlways(FileLine* fl, VAlwaysKwd keyword, AstSenTree* sensesp, AstNode* bodysp)
+	: AstNode(fl), m_keyword(keyword) {
 	addNOp1p(sensesp); addNOp2p(bodysp);
     }
     ASTNODE_NODE_FUNCS(Always, ALWAYS)
     //
+    virtual void dump(ostream& str);
     AstSenTree*	sensesp() 	const { return op1p()->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;

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);
 

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

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 {

src/V3Undriven.cpp

@@ -133,6 +133,17 @@ public:
 	    }
 	}
     }
+    bool isUsedNotDrivenBit (int bit, int width) const {
+	for (int i=0; i<width; i++) {
+	    if (bitNumOk(bit+i)
+		&& (m_usedWhole || m_flags[(bit+i)*FLAGS_PER_BIT + FLAG_USED])
+		&& !(m_drivenWhole || m_flags[(bit+i)*FLAGS_PER_BIT + FLAG_DRIVEN])) return true;
+	}
+	return false;
+    }
+    bool isUsedNotDrivenAny () const {
+	return isUsedNotDrivenBit(0, m_flags.size()/FLAGS_PER_BIT);
+    }
     bool unusedMatch(AstVar* nodep) {
 	const char* regexpp = v3Global.opt.unusedRegexp().c_str();
 	if (!regexpp || !*regexpp) return false;
@@ -213,11 +224,13 @@ private:
     // NODE STATE
     // AstVar::user1p		-> UndrivenVar* for usage var, 0=not set yet
     AstUser1InUse	m_inuser1;
+    AstUser2InUse	m_inuser2;
 
     // STATE
-    vector<UndrivenVarEntry*>	m_entryps;	// Nodes to delete when we are finished
+    vector<UndrivenVarEntry*>	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) {
+    UndrivenVarEntry* getEntryp(AstVar* nodep, int which_user) {
-	if (!nodep->user1p()) {
+	if (!(which_user==1 ? nodep->user1p() : nodep->user2p())) {
 	    UndrivenVarEntry* entryp = new UndrivenVarEntry (nodep);
-	    m_entryps.push_back(entryp);
+	    //UINFO(9," Associate u="<<which_user<<" "<<(void*)this<<" "<<nodep->name()<<endl);
-	    nodep->user1p(entryp);
+	    m_entryps[which_user].push_back(entryp);
+	    if (which_user==1) nodep->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: "<<nodep->prettyName());
+	    varp->fileline()->modifyWarnOff(V3ErrorCode::ALWCOMBORDER, true);  // Complain just once for any usage
+	}
+    }
+
     // VISITORS
     virtual void visit(AstVar* nodep, AstNUser*) {
-	UndrivenVarEntry* entryp = getEntryp (nodep);
+	for (int usr=1; usr<(m_alwaysp?3:2); ++usr) {
-	if (nodep->isInput()
+	    UndrivenVarEntry* entryp = getEntryp (nodep, usr);
-	    || nodep->isSigPublic() || nodep->isSigUserRWPublic()
+	    if (nodep->isInput()
-	    || (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) {
+		|| nodep->isSigPublic() || nodep->isSigUserRWPublic()
-	    entryp->drivenWhole();
+		|| (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) {
-	}
+		entryp->drivenWhole();
-	if (nodep->isOutput()
+	    }
-	    || nodep->isSigPublic() || nodep->isSigUserRWPublic()
+	    if (nodep->isOutput()
-	    || nodep->isSigUserRdPublic()
+		|| nodep->isSigPublic() || nodep->isSigUserRWPublic()
-	    || (m_taskp && (m_taskp->dpiImport() || m_taskp->dpiExport()))) {
+		|| nodep->isSigUserRdPublic()
-	    entryp->usedWhole();
+		|| (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());
+	    for (int usr=1; usr<(m_alwaysp?3:2); ++usr) {
-	    int lsb = constp->toUInt();
+		UndrivenVarEntry* entryp = getEntryp (varrefp->varp(), usr);
-	    if (m_markBoth || varrefp->lvalue()) entryp->drivenBit(lsb, nodep->width());
+		int lsb = constp->toUInt();
-	    if (m_markBoth || !varrefp->lvalue()) entryp->usedBit(lsb, nodep->width());
+		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<<endl);
+			warnAlwCombOrder(varrefp);
+		    }
+		    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());
+	for (int usr=1; usr<(m_alwaysp?3:2); ++usr) {
-	bool fdrv = nodep->lvalue() && nodep->varp()->attrFileDescr();  // FD's are also being read from
+	    UndrivenVarEntry* entryp = getEntryp (nodep->varp(), usr);
-	if (m_markBoth || nodep->lvalue()) entryp->drivenWhole();
+	    bool fdrv = nodep->lvalue() && nodep->varp()->attrFileDescr();  // FD's are also being read from
-	if (m_markBoth || !nodep->lvalue() || fdrv) entryp->usedWhole();
+	    if (m_markBoth || nodep->lvalue()) {
+		if (usr==2 && m_alwaysp && entryp->isUsedNotDrivenAny()) {
+		    UINFO(9," Full bus.  Entryp="<<(void*)entryp<<endl);
+		    warnAlwCombOrder(nodep);
+		}
+		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,"   "<<nodep<<endl);
+	    if (nodep->keyword() == VAlwaysKwd::ALWAYS_COMB) m_alwaysp = nodep;
+	    else m_alwaysp = NULL;
+	    nodep->iterateChildren(*this);
+	    if (nodep->keyword() == VAlwaysKwd::ALWAYS_COMB) UINFO(9,"   Done "<<nodep<<endl);
+	}
+	m_alwaysp = prevAlwp;
+    }
+
     virtual void visit(AstNodeFTask* nodep, AstNUser*) {
 	AstNodeFTask* prevTaskp = m_taskp;
 	m_taskp = nodep;
@@ -315,12 +372,17 @@ public:
     UndrivenVisitor(AstNetlist* nodep) {
 	m_markBoth = false;
 	m_taskp = NULL;
+	m_alwaysp = NULL;
 	nodep->accept(*this);
     }
     virtual ~UndrivenVisitor() {
-	for (vector<UndrivenVarEntry*>::iterator it = m_entryps.begin(); it != m_entryps.end(); ++it) {
+	for (vector<UndrivenVarEntry*>::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<UndrivenVarEntry*>::iterator it = m_entryps[usr].begin(); it != m_entryps[usr].end(); ++it) {
+		delete (*it);
+	    }
 	}
     }
 };

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_comb"		{ FL; return yALWAYS_COMB; }
-  "always_ff"		{ FL; return yALWAYS; }
+  "always_ff"		{ FL; return yALWAYS_FF; }
-  "always_latch"	{ FL; return yALWAYS; }
+  "always_latch"	{ FL; return yALWAYS_LATCH; }
   "bind"		{ FL; return yBIND; }
   "bit"			{ FL; return yBIT; }
   "break"		{ FL; return yBREAK; }

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<fl>		yALWAYS		"always"
+%token<fl>		yALWAYS_FF	"always_ff"
+%token<fl>		yALWAYS_COMB	"always_comb"
+%token<fl>		yALWAYS_LATCH	"always_latch"
 %token<fl>		yAND		"and"
 %token<fl>		yASSERT		"assert"
 %token<fl>		yASSIGN		"assign"
@@ -1549,7 +1552,10 @@ module_common_item<nodep>:	// ==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

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;

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