diff --git a/Changes b/Changes index 202e1e3b9..ba4c752ba 100644 --- a/Changes +++ b/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 ========================== diff --git a/src/Makefile_obj.in b/src/Makefile_obj.in index 4d4f12ef1..9eb16383d 100644 --- a/src/Makefile_obj.in +++ b/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 diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index 213ddd3fb..2f1467f03 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -213,7 +213,7 @@ string AstNode::prettyTypeName() const { // Insertion inline void AstNode::debugTreeChange(const AstNode* nodep, const char* prefix, int lineno, - bool next){ + bool next) { #ifdef VL_DEBUG // Called on all major tree changers. // Only for use for those really nasty bugs relating to internals @@ -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* 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* nodep, AstNode* newp) { if (!newp) return nodep; return addNext(nodep, newp); } diff --git a/src/V3Ast.h b/src/V3Ast.h index a1d057db8..4be593993 100644 --- a/src/V3Ast.h +++ b/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); - // Returns nodep, adds newp (maybe nullptr) to end of nodep's list - static AstNode* addNextNull(AstNode* nodep, AstNode* newp); - inline AstNode* addNext(AstNode* newp) { return addNext(this, newp); } - inline AstNode* addNextNull(AstNode* newp) { return addNextNull(this, newp); } - void addNextHere(AstNode* newp); // Insert newp at this->nextp - void addPrev(AstNode* newp) { - replaceWith(newp); - newp->addNext(this); + template + static T_NodeResult* addNext(T_NodeResult* nodep, T_NodeNext* newp) { + static_assert(std::is_base_of::value, + "'T_NodeResult' must be a subtype of AstNode"); + static_assert(std::is_base_of::value, + "'T_NodeNext' must be a subtype of 'T_NodeResult'"); + return static_cast(addNext(nodep, newp)); } + // Returns nodep, adds newp (maybe nullptr) to end of nodep's list + template + static T_NodeResult* addNextNull(T_NodeResult* nodep, T_NodeNext* newp) { + static_assert(std::is_base_of::value, + "'T_NodeResult' must be a subtype of AstNode"); + static_assert(std::is_base_of::value, + "'T_NodeNext' must be a subtype of 'T_NodeResult'"); + return static_cast(addNextNull(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* nodep, AstNode* newp); +template <> +AstNode* AstNode::addNextNull(AstNode* nodep, AstNode* newp); + // Specialisations of privateTypeTest #include "V3Ast__gen_impl.h" // From ./astgen diff --git a/src/V3AstInlines.h b/src/V3AstInlines.h index 6d8e93b35..9ad974a69 100644 --- a/src/V3AstInlines.h +++ b/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; } diff --git a/src/V3AstNodes.h b/src/V3AstNodes.h index a63576966..d2bfd630f 100644 --- a/src/V3AstNodes.h +++ b/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; } }; diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 0a5f6fb8a..c962996be 100644 --- a/src/V3Const.cpp +++ b/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); diff --git a/src/V3Gate.cpp b/src/V3Gate.cpp index a624db620..2b8b6c85f 100644 --- a/src/V3Gate.cpp +++ b/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 - // 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 + // 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(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<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& substitutions, GateDedupeVarVisitor* varVisp) { - const std::function visit - = [&substitutions, &visit, varVisp](AstNodeVarRef* nodep) -> void { + // Recursion filter holding already replaced variables + std::unordered_set replaced(substitutions.size() * 2); + + const std::function 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(visit); + + // Remove from recursion filter + replaced.erase(vscp); }; logicp->foreach(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* 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); } diff --git a/src/V3ParseGrammar.cpp b/src/V3ParseGrammar.cpp index 38bae0374..8e95d13c8 100644 --- a/src/V3ParseGrammar.cpp +++ b/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(); diff --git a/src/V3SchedAcyclic.cpp b/src/V3SchedAcyclic.cpp index 2ece93f73..ddce7539e 100644 --- a/src/V3SchedAcyclic.cpp +++ b/src/V3SchedAcyclic.cpp @@ -372,7 +372,7 @@ LogicByScope fixCuts(AstNetlist* netlistp, const std::vector& 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 diff --git a/src/V3Slice.cpp b/src/V3Slice.cpp index 94a70fe99..b143373f2 100644 --- a/src/V3Slice.cpp +++ b/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 - (cloneAndSel(nodep->lhsp(), elements, offset), - cloneAndSel(nodep->rhsp(), elements, offset)); + AstNodeAssign* const newp + = VN_AS(nodep->cloneType(cloneAndSel(nodep->lhsp(), elements, offset), + cloneAndSel(nodep->rhsp(), elements, offset)), + NodeAssign); if (debug() >= 9) newp->dumpTree(cout, "-new "); newlistp = AstNode::addNextNull(newlistp, newp); } diff --git a/src/V3TraceDecl.cpp b/src/V3TraceDecl.cpp index 6fc9a8726..4a596e434 100644 --- a/src/V3TraceDecl.cpp +++ b/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}; - } + 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 diff --git a/src/V3Undriven.cpp b/src/V3Undriven.cpp index 0732dafa1..4f6374cde 100644 --- a/src/V3Undriven.cpp +++ b/src/V3Undriven.cpp @@ -30,6 +30,7 @@ #include "V3Ast.h" #include "V3Global.h" +#include "V3Stats.h" #include "V3String.h" #include @@ -475,4 +476,5 @@ public: void V3Undriven::undrivenAll(AstNetlist* nodep) { UINFO(2, __FUNCTION__ << ": " << endl); { UndrivenVisitor{nodep}; } + if (v3Global.opt.stats()) V3Stats::statsStage("undriven"); } diff --git a/src/verilog.y b/src/verilog.y index 232507119..3d9cbb88e 100644 --- a/src/verilog.y +++ b/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: // 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(new AstEmpty{$3}), $4); $$ = new AstSelLoopVars($2, $1, $4); } ; // VarRef without any dots or vectorizaion diff --git a/test_regress/t/t_gate_loop.pl b/test_regress/t/t_gate_loop.pl new file mode 100755 index 000000000..1d9caab1c --- /dev/null +++ b/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; diff --git a/test_regress/t/t_gate_loop.v b/test_regress/t/t_gate_loop.v new file mode 100644 index 000000000..13ce7c416 --- /dev/null +++ b/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