Fix internal error on gate optimization of assign, bug1475.

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
 

src/V3Ast.h

@@ -61,6 +61,9 @@ typedef std::set<int> 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 {

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<AstNode*> 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<<endl);
+        // We could update the user3p and user5p but the resulting node
+        // still has incorrect hash.  We really need to remove all hash on
+        // the whole hash entry tree involving the replaced node and
+        // rehash.  That's complicated and this is rare, so just remove it
+        // from consideration.
+        m_nodeDeleteds.insert(oldp);
+    }
+    bool isReplaced(AstNode* nodep) {
+        // Assignment may have been hashReplaced, if so consider non-match (effectively removed)
+        UASSERT_OBJ(!VN_IS(nodep, NodeAssign), nodep, "Dedup attempt on non-assign");
+        AstNode* extra1p = nodep->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<<endl);
+            return false;
+        }
         return same(node1p->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<<endl);
+    if (m_varVisp) {
+        m_varVisp->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<GateLogicVertex*>(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<GateVarVertex*>(itp)) {

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

src/V3Hashed.h

@@ -53,8 +53,8 @@ class V3Hashed : public VHashedBase {
     AstUser4InUse m_inuser4;
 
     // TYPES
-    typedef std::multimap<V3Hash,AstNode*> HashMmap;
 public:
+    typedef std::multimap<V3Hash,AstNode*> 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

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;

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