From a95f58749f0af8a6e46163119feebc80acdddd6a Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sat, 3 Aug 2019 21:49:39 -0400 Subject: [PATCH] Fix internal error on gate optimization of assign, bug1475. --- Changes | 2 + src/V3Ast.h | 3 ++ src/V3Gate.cpp | 92 +++++++++++++++++++++++++++++++-- src/V3Hashed.cpp | 7 +++ src/V3Hashed.h | 3 +- test_regress/t/t_gate_delref.pl | 16 ++++++ test_regress/t/t_gate_delref.v | 48 +++++++++++++++++ 7 files changed, 167 insertions(+), 4 deletions(-) create mode 100755 test_regress/t/t_gate_delref.pl create mode 100644 test_regress/t/t_gate_delref.v diff --git a/Changes b/Changes index c2b1f8add..5fc677131 100644 --- a/Changes +++ b/Changes @@ -22,6 +22,8 @@ The contributors that suggested a given feature are shown in []. Thanks! **** Fix enum values not being sized based on parent, bug1442. [Dan Petrisko] +**** Fix internal error on gate optimization of assign, bug1475. [Oyvind Harboe] + * Verilator 4.016 2019-06-16 diff --git a/src/V3Ast.h b/src/V3Ast.h index 6e6cd3a4b..b592c6849 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -61,6 +61,9 @@ typedef std::set MTaskIdSet; // Set of mtaskIds for Var sorting #define VN_CAST(nodep,nodetypename) (AstNode::privateCast ## nodetypename(nodep)) #define VN_CAST_CONST(nodep,nodetypename) (AstNode::privateConstCast ## nodetypename(nodep) ) +// (V)erilator (N)ode deleted: Reference to deleted child (for assertions only) +#define VN_DELETED(nodep) VL_UNLIKELY((vluint64_t)(nodep) == 0x1) + //###################################################################### class AstType { diff --git a/src/V3Gate.cpp b/src/V3Gate.cpp index f17027044..617c47fb4 100644 --- a/src/V3Gate.cpp +++ b/src/V3Gate.cpp @@ -835,6 +835,8 @@ void GateVisitor::warnSignals() { //###################################################################### // Push constant into expressions and reevaluate +class GateDedupeVarVisitor; + class GateElimVisitor : public GateBaseVisitor { private: // NODE STATE @@ -842,6 +844,11 @@ private: AstVarScope* m_elimVarScp; // Variable being eliminated AstNode* m_replaceTreep; // What to replace the variable with bool m_didReplace; // Did we do any replacements + GateDedupeVarVisitor* m_varVisp; // Callback to keep hash up to date + + // METHODS + void hashReplace(AstNode* oldp, AstNode* newp); + // VISITORS virtual void visit(AstNodeVarRef* nodep) { if (nodep->varScopep() == m_elimVarScp) { @@ -865,6 +872,7 @@ private: if (VN_IS(substp, VarRef)) substp->fileline(nodep->fileline()); // Make the substp an rvalue like nodep. This facilitate the hashing in dedupe. if (AstNodeVarRef* varrefp = VN_CAST(substp, NodeVarRef)) varrefp->lvalue(false); + hashReplace(nodep, substp); nodep->replaceWith(substp); nodep->deleteTree(); VL_DANGLING(nodep); } @@ -875,10 +883,12 @@ private: public: // CONSTUCTORS virtual ~GateElimVisitor() {} - GateElimVisitor(AstNode* nodep, AstVarScope* varscp, AstNode* replaceTreep) { + GateElimVisitor(AstNode* nodep, AstVarScope* varscp, AstNode* replaceTreep, + GateDedupeVarVisitor* varVisp) { m_didReplace = false; m_elimVarScp = varscp; m_replaceTreep = replaceTreep; + m_varVisp = varVisp; iterate(nodep); } bool didReplace() const { return m_didReplace; } @@ -886,7 +896,7 @@ public: void GateVisitor::optimizeElimVar(AstVarScope* varscp, AstNode* substp, AstNode* consumerp) { if (debug()>=5) consumerp->dumpTree(cout, "\telimUsePre: "); - GateElimVisitor elimVisitor (consumerp, varscp, substp); + GateElimVisitor elimVisitor (consumerp, varscp, substp, NULL); if (elimVisitor.didReplace()) { if (debug()>=9) consumerp->dumpTree(cout, "\telimUseCns: "); //Caution: Can't let V3Const change our handle to consumerp, such as by @@ -903,16 +913,27 @@ void GateVisitor::optimizeElimVar(AstVarScope* varscp, AstNode* substp, AstNode* // Auxiliary hash class for GateDedupeVarVisitor class GateDedupeHash : public V3HashedUserSame { +public: + // TYPES + typedef std::set NodeSet; + private: // NODE STATE // Ast*::user2p -> parent AstNodeAssign* for this rhsp // Ast*::user3p -> AstActive* of assign, for isSame() in test for duplicate + // Set to NULL if this assign's tree was later replaced // Ast*::user5p -> AstNode* of assign if condition, for isSame() in test for duplicate + // Set to NULL if this assign's tree was later replaced + // AstUser1InUse m_inuser1; (Allocated for use in GateVisitor) // AstUser2InUse m_inuser2; (Allocated for use in GateVisitor) AstUser3InUse m_inuser3; // AstUser4InUse m_inuser4; (Allocated for use in V3Hashed) AstUser5InUse m_inuser5; + V3Hashed m_hashed; // Hash, contains rhs of assigns + NodeSet m_nodeDeleteds; // Any node in this hash was deleted + + VL_DEBUG_FUNC; // Declare debug() void hash(AstNode* nodep) { // !NULL && the object is hashable @@ -930,13 +951,46 @@ private: return node1p == node2p || sameHash(node1p, node2p); } public: + GateDedupeHash() { } + ~GateDedupeHash() { + if (v3Global.opt.debugCheck()) check(); + } + + // About to replace a node; any node we're tracking refers to oldp, change it to newp. + // This might be a variable on the lhs of the duplicate tree, + // or could be a rhs variable in a tree we're not replacing (or not yet anyways) + void hashReplace(AstNode* oldp, AstNode* newp) { + UINFO(9,"replacing "<<(void*)oldp<<" with "<<(void*)newp<user3p(); + AstNode* extra2p = nodep->user5p(); + return ((extra1p && m_nodeDeleteds.find(extra1p) != m_nodeDeleteds.end()) + || (extra2p && m_nodeDeleteds.find(extra2p) != m_nodeDeleteds.end())); + } + + // Callback from V3Hashed::findDuplicate bool isSame(AstNode* node1p, AstNode* node2p) { + // Assignment may have been hashReplaced, if so consider non-match (effectively removed) + if (isReplaced(node1p) || isReplaced(node2p)) { + //UINFO(9, "isSame hit on replaced "<<(void*)node1p<<" "<<(void*)node2p<user3p(), node2p->user3p()) && same(node1p->user5p(), node2p->user5p()) && node1p->user2p()->type() == node2p->user2p()->type(); } AstNodeAssign* hashAndFindDupe(AstNodeAssign* assignp, AstNode* extra1p, AstNode* extra2p) { + // Legal for extra1p/2p to be NULL, we'll compare with other assigns with extras also NULL AstNode *rhsp = assignp->rhsp(); rhsp->user2p(assignp); rhsp->user3p(extra1p); @@ -954,8 +1008,26 @@ public: m_hashed.erase(inserted); return VN_CAST(m_hashed.iteratorNodep(dupit)->user2p(), NodeAssign); } + // Retain new inserted information return NULL; } + + void check() { + m_hashed.check(); + for (V3Hashed::HashMmap::iterator it = m_hashed.begin(); it != m_hashed.end(); ++it) { + AstNode* nodep = it->second; + AstNode* activep = nodep->user3p(); + AstNode* condVarp = nodep->user5p(); + if (!isReplaced(nodep)) { + // This class won't break if activep isn't an active, or + // ifVar isn't a var, but this is checking the caller's construction. + UASSERT_OBJ(!activep || (!VN_DELETED(activep) && VN_IS(activep, Active)), + nodep, "V3Hashed check failed, lost active pointer"); + UASSERT_OBJ(!condVarp || !VN_DELETED(condVarp), + nodep, "V3Hashed check failed, lost if pointer"); + } + } + } }; //###################################################################### @@ -1032,6 +1104,7 @@ public: m_always = false; m_dedupable = true; } + ~GateDedupeVarVisitor() { } // PUBLIC METHODS AstNodeVarRef* findDupe(AstNode* nodep, AstVarScope* consumerVarScopep, AstActive* activep) { m_assignp = NULL; @@ -1052,8 +1125,20 @@ public: } return NULL; } + void hashReplace(AstNode* oldp, AstNode* newp) { + m_ghash.hashReplace(oldp, newp); + } }; +//###################################################################### + +void GateElimVisitor::hashReplace(AstNode* oldp, AstNode* newp) { + UINFO(9,"hashReplace "<<(void*)oldp<<" -> "<<(void*)newp<hashReplace(oldp, newp); + } +} + //###################################################################### // Recurse through the graph, looking for duplicate expressions on the rhs of an assign @@ -1093,7 +1178,7 @@ private: GateLogicVertex* consumeVertexp = dynamic_cast(outedgep->top()); AstNode* consumerp = consumeVertexp->nodep(); - GateElimVisitor elimVisitor(consumerp, vvertexp->varScp(), dupVarRefp); + GateElimVisitor elimVisitor(consumerp, vvertexp->varScp(), dupVarRefp, &m_varVisitor); outedgep = outedgep->relinkFromp(dupVvertexp); } @@ -1299,6 +1384,7 @@ public: //---------------------------------------------------------------------- void GateVisitor::mergeAssigns() { + UINFO(6, "mergeAssigns\n"); GateMergeAssignsGraphVisitor merger(&m_graph); for (V3GraphVertex* itp = m_graph.verticesBeginp(); itp; itp=itp->verticesNextp()) { if (GateVarVertex* vvertexp = dynamic_cast(itp)) { diff --git a/src/V3Hashed.cpp b/src/V3Hashed.cpp index 6c5a3353a..732dde45e 100644 --- a/src/V3Hashed.cpp +++ b/src/V3Hashed.cpp @@ -145,6 +145,13 @@ void V3Hashed::erase(iterator it) { nodep->user4p(NULL); // So we don't allow removeNode again } +void V3Hashed::check() { + for (HashMmap::iterator it = begin(); it != end(); ++it) { + AstNode* nodep = it->second; + UASSERT_OBJ(nodep->user4p(), nodep, "V3Hashed check failed, non-hashed node"); + } +} + void V3Hashed::dumpFilePrefixed(const string& nameComment, bool tree) { if (v3Global.opt.dumpTree()) { dumpFile(v3Global.debugFilename(nameComment)+".hash", tree); diff --git a/src/V3Hashed.h b/src/V3Hashed.h index 8f14940aa..e2a628c7d 100644 --- a/src/V3Hashed.h +++ b/src/V3Hashed.h @@ -53,8 +53,8 @@ class V3Hashed : public VHashedBase { AstUser4InUse m_inuser4; // TYPES - typedef std::multimap HashMmap; public: + typedef std::multimap HashMmap; typedef HashMmap::iterator iterator; private: // MEMBERS @@ -72,6 +72,7 @@ public: // METHODS void clear() { m_hashMmap.clear(); AstNode::user4ClearTree(); } + void check(); // Check assertions on structure iterator hashAndInsert(AstNode* nodep); // Hash the node, and insert into map. Return iterator to inserted void hash(AstNode* nodep); // Only hash the node bool sameNodes(AstNode* node1p, AstNode* node2p); // After hashing, and tell if identical diff --git a/test_regress/t/t_gate_delref.pl b/test_regress/t/t_gate_delref.pl new file mode 100755 index 000000000..683765bd3 --- /dev/null +++ b/test_regress/t/t_gate_delref.pl @@ -0,0 +1,16 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2004 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. + +scenarios(simulator => 1); + +compile( + ); + +ok(1); +1; diff --git a/test_regress/t/t_gate_delref.v b/test_regress/t/t_gate_delref.v new file mode 100644 index 000000000..154559b72 --- /dev/null +++ b/test_regress/t/t_gate_delref.v @@ -0,0 +1,48 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2019 by Wilson Snyder. + +// bug1475 +module t (/*AUTOARG*/ + // Outputs + ID_45, IDa_f4c, + // Inputs + clk, ID_d9f, IDa_657, ID_477 + ); + input clk; + output reg ID_45; + input ID_d9f; + input IDa_657; + output reg IDa_f4c; + + reg ID_13; + input ID_477; + reg ID_489; + reg ID_8d1; + reg IDa_183; + reg IDa_91c; + reg IDa_a96; + reg IDa_d6b; + reg IDa_eb9; + wire ID_fc8 = ID_d9f & ID_13; //<< + wire ID_254 = ID_d9f & ID_13; + wire ID_f40 = ID_fc8 ? ID_8d1 : 0; + wire ID_f4c = ID_fc8 ? 0 : ID_477; + wire ID_442 = IDa_91c; + wire ID_825 = ID_489; + always @(posedge clk) begin + ID_13 <= ID_f40; + ID_8d1 <= IDa_eb9; + ID_489 <= ID_442; + ID_45 <= ID_825; + IDa_d6b <= IDa_a96; + IDa_f4c <= ID_f4c; + if (ID_254) begin + IDa_91c <= IDa_d6b; + IDa_183 <= IDa_657; + IDa_a96 <= IDa_657; + IDa_eb9 <= IDa_183; + end + end +endmodule