Merge branch 'master' into develop-v5

Changes

@@ -26,6 +26,8 @@ Verilator 4.227 devel
 
 **Minor:**
 
+Fix crash in gate optimization of circular logic (#3543). [Bill Flynn]
+
 
 Verilator 4.226 2022-08-31
 ==========================

src/Makefile_obj.in

@@ -79,7 +79,7 @@ ifeq ($(VL_NOOPT),1)
 CPPFLAGS += -O0
 else ifeq ($(VL_DEBUG),)
 # Optimize
-CPPFLAGS += -O2
+CPPFLAGS += -O3
 else
 # Debug
 CPPFLAGS += @CFG_CXXFLAGS_DEBUG@ -DVL_DEBUG -D_GLIBCXX_DEBUG

src/V3Ast.cpp

@@ -234,7 +234,8 @@ inline void AstNode::debugTreeChange(const AstNode* nodep, const char* prefix, i
 #endif
 }
 
-AstNode* AstNode::addNext(AstNode* nodep, AstNode* newp) {
+template <>
+AstNode* AstNode::addNext<AstNode, AstNode>(AstNode* nodep, AstNode* newp) {
     // Add to m_nextp, returns this
     UDEBUGONLY(UASSERT_OBJ(newp, nodep, "Null item passed to addNext"););
     debugTreeChange(nodep, "-addNextThs: ", __LINE__, false);
@@ -272,7 +273,8 @@ AstNode* AstNode::addNext(AstNode* nodep, AstNode* newp) {
     return nodep;
 }
 
-AstNode* AstNode::addNextNull(AstNode* nodep, AstNode* newp) {
+template <>
+AstNode* AstNode::addNextNull<AstNode, AstNode>(AstNode* nodep, AstNode* newp) {
     if (!newp) return nodep;
     return addNext(nodep, newp);
 }

src/V3Ast.h

@@ -1813,16 +1813,27 @@ public:
 
     // METHODS - Tree modifications
     // Returns nodep, adds newp to end of nodep's list
-    static AstNode* addNext(AstNode* nodep, AstNode* newp);
+    template <typename T_NodeResult, typename T_NodeNext>
-    // Returns nodep, adds newp (maybe nullptr) to end of nodep's list
+    static T_NodeResult* addNext(T_NodeResult* nodep, T_NodeNext* newp) {
-    static AstNode* addNextNull(AstNode* nodep, AstNode* newp);
+        static_assert(std::is_base_of<AstNode, T_NodeResult>::value,
-    inline AstNode* addNext(AstNode* newp) { return addNext(this, newp); }
+                      "'T_NodeResult' must be a subtype of AstNode");
-    inline AstNode* addNextNull(AstNode* newp) { return addNextNull(this, newp); }
+        static_assert(std::is_base_of<T_NodeResult, T_NodeNext>::value,
-    void addNextHere(AstNode* newp);  // Insert newp at this->nextp
+                      "'T_NodeNext' must be a subtype of 'T_NodeResult'");
-    void addPrev(AstNode* newp) {
+        return static_cast<T_NodeResult*>(addNext<AstNode, AstNode>(nodep, newp));
-        replaceWith(newp);
-        newp->addNext(this);
     }
+    // Returns nodep, adds newp (maybe nullptr) to end of nodep's list
+    template <typename T_NodeResult, typename T_NodeNext>
+    static T_NodeResult* addNextNull(T_NodeResult* nodep, T_NodeNext* newp) {
+        static_assert(std::is_base_of<AstNode, T_NodeResult>::value,
+                      "'T_NodeResult' must be a subtype of AstNode");
+        static_assert(std::is_base_of<T_NodeResult, T_NodeNext>::value,
+                      "'T_NodeNext' must be a subtype of 'T_NodeResult'");
+        return static_cast<T_NodeResult*>(addNextNull<AstNode, AstNode>(nodep, newp));
+    }
+    inline AstNode* addNext(AstNode* newp);
+    inline AstNode* addNextNull(AstNode* newp);
+    void addNextHere(AstNode* newp);  // Insert newp at this->nextp
+    inline void addPrev(AstNode* newp);
     void addHereThisAsNext(AstNode* newp);  // Adds at old place of this, this becomes next
     void replaceWith(AstNode* newp);  // Replace current node in tree with new node
     AstNode* unlinkFrBack(VNRelinker* linkerp
@@ -2112,6 +2123,12 @@ public:
     }
 };
 
+// Forward declarations of specializations defined in V3Ast.cpp
+template <>
+AstNode* AstNode::addNext<AstNode, AstNode>(AstNode* nodep, AstNode* newp);
+template <>
+AstNode* AstNode::addNextNull<AstNode, AstNode>(AstNode* nodep, AstNode* newp);
+
 // Specialisations of privateTypeTest
 #include "V3Ast__gen_impl.h"  // From ./astgen
 

src/V3AstInlines.h

@@ -23,7 +23,14 @@
 #endif
 
 //######################################################################
-// Inline ACCESSORS
+// Inline METHODS
+
+inline AstNode* AstNode::addNext(AstNode* newp) { return addNext(this, newp); }
+inline AstNode* AstNode::addNextNull(AstNode* newp) { return addNextNull(this, newp); }
+inline void AstNode::addPrev(AstNode* newp) {
+    replaceWith(newp);
+    newp->addNext(this);
+}
 
 inline int AstNode::width() const { return dtypep() ? dtypep()->width() : 0; }
 inline int AstNode::widthMin() const { return dtypep() ? dtypep()->widthMin() : 0; }

src/V3AstNodes.h

@@ -2564,9 +2564,6 @@ public:
     AstVar* varp() const { return m_varp; }  // [After Link] Pointer to variable
     AstScope* scopep() const { return m_scopep; }  // Pointer to scope it's under
     void scopep(AstScope* nodep) { m_scopep = nodep; }
-    // op1 = Calculation of value of variable, nullptr=complicated
-    AstNode* valuep() const { return op1p(); }
-    void valuep(AstNode* valuep) { addOp1p(valuep); }
     bool isTrace() const { return m_trace; }
     void trace(bool flag) { m_trace = flag; }
 };

src/V3Const.cpp

@@ -2079,7 +2079,7 @@ private:
             AstSel* const sel2p = new AstSel(conp->fileline(), rhs2p, lsb2, msb2 - lsb2 + 1);
             // Make new assigns of same flavor as old one
             //*** Not cloneTree; just one node.
-            AstNode* newp = nullptr;
+            AstNodeAssign* newp = nullptr;
             if (!need_temp) {
                 AstNodeAssign* const asn1ap = VN_AS(nodep->cloneType(lc1p, sel1p), NodeAssign);
                 AstNodeAssign* const asn2ap = VN_AS(nodep->cloneType(lc2p, sel2p), NodeAssign);

src/V3Gate.cpp

@@ -418,7 +418,6 @@ private:
     void consumedMark();
     void consumedMarkRecurse(GateEitherVertex* vertexp);
     void consumedMove();
-    void replaceAssigns();
     void dedupe();
     void mergeAssigns();
     void decomposeClkVectors();
@@ -455,7 +454,6 @@ private:
         m_graph.dumpDotFilePrefixed("gate_opt");
         // Rewrite assignments
         consumedMove();
-        replaceAssigns();
     }
     virtual void visit(AstNodeModule* nodep) override {
         VL_RESTORER(m_modp);
@@ -665,14 +663,7 @@ void GateVisitor::optimizeSignals(bool allowMultiIn) {
             while (V3GraphEdge* const edgep = vvertexp->inBeginp()) {
                 VL_DO_DANGLING(edgep->unlinkDelete(), edgep);
             }
-            // Clone tree so we remember it for tracing, and keep the pointer
+            // Mark the vertex so we don't mark it as being unconsumed in the next step
-            // to the "ALWAYS" part of the tree as part of this statement
-            // That way if a later signal optimization that
-            // retained a pointer to the always can
-            // optimize it further
-            VL_DO_DANGLING(vvertexp->varScp()->valuep(logicp->unlinkFrBack()), logicp);
-            // Mark the vertex so we don't mark it as being
-            // unconsumed in the next step
             vvertexp->user(true);
             logicVertexp->user(true);
         }
@@ -704,45 +695,6 @@ bool GateVisitor::elimLogicOkOutputs(GateLogicVertex* consumeVertexp,
     return true;
 }
 
-void GateVisitor::replaceAssigns() {
-    for (V3GraphVertex* itp = m_graph.verticesBeginp(); itp; itp = itp->verticesNextp()) {
-        if (const GateVarVertex* const vvertexp = dynamic_cast<GateVarVertex*>(itp)) {
-            // Take the Comments/assigns that were moved to the VarScope and change them to a
-            // simple value assignment
-            const AstVarScope* const vscp = vvertexp->varScp();
-            if (vscp->valuep() && !VN_IS(vscp->valuep(), NodeMath)) {
-                // if (debug() > 9) vscp->dumpTree(cout, "-vscPre:  ");
-                while (AstNode* delp = VN_CAST(vscp->valuep(), Comment)) {
-                    VL_DO_DANGLING(delp->unlinkFrBack()->deleteTree(), delp);
-                }
-                if (AstInitial* const delp = VN_CAST(vscp->valuep(), Initial)) {
-                    AstNode* const bodyp = delp->bodysp();
-                    bodyp->unlinkFrBackWithNext();
-                    delp->replaceWith(bodyp);
-                    VL_DO_DANGLING(delp->deleteTree(), delp);
-                }
-                if (AstAlways* const delp = VN_CAST(vscp->valuep(), Always)) {
-                    AstNode* const bodyp = delp->bodysp();
-                    bodyp->unlinkFrBackWithNext();
-                    delp->replaceWith(bodyp);
-                    VL_DO_DANGLING(delp->deleteTree(), delp);
-                }
-                if (AstNodeAssign* const delp = VN_CAST(vscp->valuep(), NodeAssign)) {
-                    AstNode* const rhsp = delp->rhsp();
-                    rhsp->unlinkFrBack();
-                    delp->replaceWith(rhsp);
-                    VL_DO_DANGLING(delp->deleteTree(), delp);
-                }
-                // if (debug() > 9) {vscp->dumpTree(cout, "-vscDone: "); cout<<endl;}
-                if (!VN_IS(vscp->valuep(), NodeMath) || vscp->valuep()->nextp()) {
-                    vscp->dumpTree(std::cerr, "vscStrange: ");
-                    vscp->v3fatalSrc("Value of varscope not mathematical");
-                }
-            }
-        }
-    }
-}
-
 //----------------------------------------------------------------------
 
 void GateVisitor::consumedMark() {
@@ -1044,11 +996,18 @@ static void eliminate(AstNode* logicp,
                       const std::unordered_map<AstVarScope*, AstNode*>& substitutions,
                       GateDedupeVarVisitor* varVisp) {
 
-    const std::function<void(AstNodeVarRef*)> visit
+    // Recursion filter holding already replaced variables
-        = [&substitutions, &visit, varVisp](AstNodeVarRef* nodep) -> void {
+    std::unordered_set<const AstVarScope*> replaced(substitutions.size() * 2);
+
+    const std::function<void(AstNodeVarRef*)> visit = [&, varVisp](AstNodeVarRef* nodep) -> void {
         // See if this variable has a substitution
-        const auto& it = substitutions.find(nodep->varScopep());
+        AstVarScope* const vscp = nodep->varScopep();
+        const auto& it = substitutions.find(vscp);
         if (it == substitutions.end()) return;
+
+        // Do not substitute circular logic
+        if (!replaced.insert(vscp).second) return;
+
         AstNode* const substp = it->second;
 
         // Substitute in the new tree
@@ -1075,6 +1034,9 @@ static void eliminate(AstNode* logicp,
         VL_DO_DANGLING(nodep->deleteTree(), nodep);
         // Recursively substitute the new tree
         newp->foreach<AstNodeVarRef>(visit);
+
+        // Remove from recursion filter
+        replaced.erase(vscp);
     };
 
     logicp->foreach<AstNodeVarRef>(visit);
@@ -1143,11 +1105,6 @@ private:
                     while (V3GraphEdge* const inedgep = vvertexp->inBeginp()) {
                         VL_DO_DANGLING(inedgep->unlinkDelete(), inedgep);
                     }
-                    // replaceAssigns() does the deleteTree on lvertexNodep in a later step
-                    AstNode* lvertexNodep = lvertexp->nodep();
-                    lvertexNodep->unlinkFrBack();
-                    vvertexp->varScp()->valuep(lvertexNodep);
-                    lvertexNodep = nullptr;
                     vvertexp->user(true);
                     lvertexp->user(true);
                 }
@@ -1538,16 +1495,5 @@ void GateVisitor::decomposeClkVectors() {
 void V3Gate::gateAll(AstNetlist* nodep) {
     UINFO(2, __FUNCTION__ << ": " << endl);
     { const GateVisitor visitor{nodep}; }  // Destruct before checking
-
-    nodep->foreach<AstVarScope>([](AstVarScope* nodep) {
-        if (AstNodeAssign* const assp = VN_CAST(nodep->valuep(), NodeAssign)) {
-            UINFO(5, " Removeassign " << assp << endl);
-            AstNode* const valuep = assp->rhsp();
-            valuep->unlinkFrBack();
-            assp->replaceWith(valuep);
-            VL_DO_DANGLING(assp->deleteTree(), assp);
-        }
-    });
-
     V3Global::dumpCheckGlobalTree("gate", 0, v3Global.opt.dumpTreeLevel(__FILE__) >= 3);
 }

src/V3ParseGrammar.cpp

@@ -67,10 +67,10 @@ void V3ParseImp::parserClear() {
 //======================================================================
 // V3ParseGrammar functions requiring bison state
 
-AstNode* V3ParseGrammar::argWrapList(AstNode* nodep) {
+AstArg* V3ParseGrammar::argWrapList(AstNode* nodep) {
     // Convert list of expressions to list of arguments
     if (!nodep) return nullptr;
-    AstNode* outp = nullptr;
+    AstArg* outp = nullptr;
     AstBegin* const tempp = new AstBegin(nodep->fileline(), "[EditWrapper]", nodep);
     while (nodep) {
         AstNode* const nextp = nodep->nextp();

src/V3SchedAcyclic.cpp

@@ -372,7 +372,7 @@ LogicByScope fixCuts(AstNetlist* netlistp, const std::vector<VarVertex*>& cutVer
         for (AstVarScope* const vscp : lvtx2Cuts[lvtxp]) {
             AstVarRef* const refp = new AstVarRef{flp, vscp, VAccess::READ};
             AstSenItem* const nextp = new AstSenItem{flp, VEdgeType::ET_HYBRID, refp};
-            senItemsp = VN_AS(AstNode::addNext(senItemsp, nextp), SenItem);
+            senItemsp = AstNode::addNext(senItemsp, nextp);
         }
         AstSenTree* const senTree = new AstSenTree{flp, senItemsp};
         // Add logic to result with new sensitivity

src/V3Slice.cpp

@@ -139,12 +139,13 @@ class SliceVisitor final : public VNVisitor {
                 // Left and right could have different msb/lsbs/endianness, but #elements is common
                 // and all variables are realigned to start at zero
                 // Assign of a little endian'ed slice to a big endian one must reverse the elements
-                AstNode* newlistp = nullptr;
+                AstNodeAssign* newlistp = nullptr;
                 const int elements = arrayp->rangep()->elementsConst();
                 for (int offset = 0; offset < elements; ++offset) {
-                    AstNode* const newp = nodep->cloneType  // AstNodeAssign
+                    AstNodeAssign* const newp
-                                          (cloneAndSel(nodep->lhsp(), elements, offset),
+                        = VN_AS(nodep->cloneType(cloneAndSel(nodep->lhsp(), elements, offset),
-                                           cloneAndSel(nodep->rhsp(), elements, offset));
+                                                 cloneAndSel(nodep->rhsp(), elements, offset)),
+                                NodeAssign);
                     if (debug() >= 9) newp->dumpTree(cout, "-new ");
                     newlistp = AstNode::addNextNull(newlistp, newp);
                 }

src/V3TraceDecl.cpp

@@ -280,11 +280,7 @@ private:
                 addIgnore(ignoreReasonp);
             } else {
                 ++m_statSigs;
-                if (AstNode* const valuep = m_traVscp->valuep()) {
-                    m_traValuep = valuep->cloneTree(false);
-                } else {
                 m_traValuep = new AstVarRef{m_traVscp->fileline(), m_traVscp, VAccess::READ};
-                }
                 // Recurse into data type of the signal. The visit methods will add AstTraceDecls.
                 iterate(m_traVscp->varp()->dtypep()->skipRefToEnump());
                 // Cleanup

src/V3Undriven.cpp

@@ -30,6 +30,7 @@
 
 #include "V3Ast.h"
 #include "V3Global.h"
+#include "V3Stats.h"
 #include "V3String.h"
 
 #include <algorithm>
@@ -475,4 +476,5 @@ public:
 void V3Undriven::undrivenAll(AstNetlist* nodep) {
     UINFO(2, __FUNCTION__ << ": " << endl);
     { UndrivenVisitor{nodep}; }
+    if (v3Global.opt.stats()) V3Stats::statsStage("undriven");
 }

src/verilog.y

@@ -102,7 +102,7 @@ public:
     }
 
     // METHODS
-    AstNode* argWrapList(AstNode* nodep);
+    AstArg* argWrapList(AstNode* nodep);
     bool allTracingOn(FileLine* fl) {
         return v3Global.opt.trace() && m_tracingParse && fl->tracingOn();
     }
@@ -5100,7 +5100,7 @@ idArrayedForeach<nodep>:        // IEEE: id + select (under foreach expression)
         |       idArrayed '[' expr ',' loop_variables ']'
                         { $3 = AstNode::addNextNull($3, $5); $$ = new AstSelLoopVars($2, $1, $3); }
         |       idArrayed '[' ',' loop_variables ']'
-                        { $4 = AstNode::addNextNull(new AstEmpty{$3}, $4); $$ = new AstSelLoopVars($2, $1, $4); }
+                        { $4 = AstNode::addNextNull(static_cast<AstNode*>(new AstEmpty{$3}), $4); $$ = new AstSelLoopVars($2, $1, $4); }
         ;
 
 // VarRef without any dots or vectorizaion

test_regress/t/t_gate_loop.pl

@@ -0,0 +1,18 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2022 by Geza Lore. 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.
+# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+scenarios(simulator => 1);
+
+compile(
+    verilator_flags2 => ["-Wno-UNOPTFLAT"]
+    );
+
+ok(1);
+1;

test_regress/t/t_gate_loop.v

@@ -0,0 +1,14 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2022 by Geza Lore.
+// SPDX-License-Identifier: CC0-1.0
+
+module t;
+  wire a;
+  wire b;
+  wire c;
+  assign a = b;
+  assign b = c;
+  assign c = a;
+endmodule