From 8e8f4b1e5c14467503f0f5d6651e0cf8c7a00350 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 2 Sep 2022 16:38:06 +0100 Subject: [PATCH 1/5] Remove AstVarScope::valuep() and related code This is detritus from when V3TraceDecl used to run after V3Gate, today V3TraceDecl runs before V3Gate and this value has no function at all. No functional change intended. --- src/V3AstNodes.h | 3 --- src/V3Gate.cpp | 66 +-------------------------------------------- src/V3TraceDecl.cpp | 6 +---- 3 files changed, 2 insertions(+), 73 deletions(-) diff --git a/src/V3AstNodes.h b/src/V3AstNodes.h index 64c25e494..cef18db50 100644 --- a/src/V3AstNodes.h +++ b/src/V3AstNodes.h @@ -2553,9 +2553,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 isCircular() const { return m_circular; } void circular(bool flag) { m_circular = flag; } bool isTrace() const { return m_trace; } diff --git a/src/V3Gate.cpp b/src/V3Gate.cpp index 726bbbfa8..a4c72d5d4 100644 --- a/src/V3Gate.cpp +++ b/src/V3Gate.cpp @@ -408,7 +408,6 @@ private: void consumedMark(); void consumedMarkRecurse(GateEitherVertex* vertexp); void consumedMove(); - void replaceAssigns(); void dedupe(); void mergeAssigns(); void decomposeClkVectors(); @@ -445,7 +444,6 @@ private: m_graph.dumpDotFilePrefixed("gate_opt"); // Rewrite assignments consumedMove(); - replaceAssigns(); } virtual void visit(AstNodeModule* nodep) override { VL_RESTORER(m_modp); @@ -654,14 +652,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); } @@ -693,45 +684,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() { @@ -1132,11 +1084,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); } @@ -1527,16 +1474,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/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 From d42a2d6494d6c13b5884bd5e2b6be929310856ba Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 2 Sep 2022 19:31:13 +0100 Subject: [PATCH 2/5] Fix V3Gate crash on circular logic The recent patch to defer substitutions on V3Gate crashes on circular logic that has cycle length >= 3 with all inlineable signals (cycle length 2 is detected correctly and is not inlined). Fix by stopping recursion at the loop-back edge. Fixes #3543 --- Changes | 2 ++ src/V3Gate.cpp | 16 +++++++++++++--- test_regress/t/t_gate_loop.pl | 18 ++++++++++++++++++ test_regress/t/t_gate_loop.v | 14 ++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100755 test_regress/t/t_gate_loop.pl create mode 100644 test_regress/t/t_gate_loop.v diff --git a/Changes b/Changes index 3cb14209e..5cae1fc1c 100644 --- a/Changes +++ b/Changes @@ -13,6 +13,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/V3Gate.cpp b/src/V3Gate.cpp index a4c72d5d4..e4b2e8853 100644 --- a/src/V3Gate.cpp +++ b/src/V3Gate.cpp @@ -985,11 +985,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 @@ -1016,6 +1023,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); 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 From 937e893b6d689a641d2d0be91cfec1f40ff05dca Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 3 Sep 2022 22:10:07 +0100 Subject: [PATCH 3/5] Build verilator_bin with -O3 (#3592) This is consistently a few percent faster. --- src/Makefile_obj.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile_obj.in b/src/Makefile_obj.in index 140be6086..1496cc411 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 From fb931087abf52db1f0dbb6d3679c2b1b71cf56f3 Mon Sep 17 00:00:00 2001 From: Krzysztof Bieganski Date: Mon, 5 Sep 2022 17:20:38 +0200 Subject: [PATCH 4/5] Add stats tracking for `V3Undriven`. (#3600) Signed-off-by: Krzysztof Bieganski --- src/V3Undriven.cpp | 2 ++ 1 file changed, 2 insertions(+) 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"); } From 6b6790fc501dde4bc5d14c5b97a1a061584a2191 Mon Sep 17 00:00:00 2001 From: Krzysztof Bieganski Date: Mon, 29 Aug 2022 15:26:00 +0200 Subject: [PATCH 5/5] Preserve return type of `AstNode::addNext` via templating (#3597) No functional change intended. Signed-off-by: Krzysztof Bieganski --- src/V3Ast.cpp | 8 +++++--- src/V3Ast.h | 35 ++++++++++++++++++++++++++--------- src/V3AstInlines.h | 9 ++++++++- src/V3Const.cpp | 2 +- src/V3ParseGrammar.cpp | 4 ++-- src/V3Slice.cpp | 9 +++++---- src/verilog.y | 4 ++-- 7 files changed, 49 insertions(+), 22 deletions(-) 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 b05c43318..9e50846cd 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -1759,16 +1759,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 @@ -2056,6 +2067,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/V3Const.cpp b/src/V3Const.cpp index fb90cf866..096510af1 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/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/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/verilog.y b/src/verilog.y index 907ca05d2..3d77c680b 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -91,7 +91,7 @@ public: } // METHODS - AstNode* argWrapList(AstNode* nodep); + AstArg* argWrapList(AstNode* nodep); bool allTracingOn(FileLine* fl) { return v3Global.opt.trace() && m_tracingParse && fl->tracingOn(); } @@ -5103,7 +5103,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