Fix V3SplitVar test stability (#2408)

src/V3FileLine.h

@@ -267,6 +267,21 @@ public:
                 && m_lastLineno == rhs.m_lastLineno && m_lastColumn == rhs.m_lastColumn
                 && m_filenameno == rhs.m_filenameno && m_warnOn == rhs.m_warnOn);
     }
+    // Returns -1 if (*this) should come before rhs after sorted. 1 for the opposite case. 0 for
+    // equivalent.
+    int operatorCompare(const FileLine& rhs) const {
+        if (m_filenameno != rhs.m_filenameno) return (m_filenameno < rhs.m_filenameno) ? -1 : 1;
+        if (m_firstLineno != rhs.m_firstLineno)
+            return (m_firstLineno < rhs.m_firstLineno) ? -1 : 1;
+        if (m_firstColumn != rhs.m_firstColumn)
+            return (m_firstColumn < rhs.m_firstColumn) ? -1 : 1;
+        if (m_lastLineno != rhs.m_lastLineno) return (m_lastLineno < rhs.m_lastLineno) ? -1 : 1;
+        if (m_lastColumn != rhs.m_lastColumn) return (m_lastColumn < rhs.m_lastColumn) ? -1 : 1;
+        for (size_t i = 0; i < m_warnOn.size(); ++i) {
+            if (m_warnOn[i] != rhs.m_warnOn[i]) return (m_warnOn[i] < rhs.m_warnOn[i]) ? -1 : 1;
+        }
+        return 0;  // (*this) and rhs are equivalent
+    }
 
 private:
     string warnContext(bool secondary) const;

src/V3SplitVar.cpp

@@ -122,8 +122,6 @@
 #include <map>
 #include <set>
 #include <vector>
-#include VL_INCLUDE_UNORDERED_MAP
-#include VL_INCLUDE_UNORDERED_SET
 
 struct SplitVarImpl {
     static AstNodeAssign* newAssign(FileLine* fileline, AstNode* lhsp, AstNode* rhsp,
@@ -228,6 +226,15 @@ const char* const SplitVarImpl::notSplitMsg
 // AstVarRef    -> Create a temporary variable and refer the variable
 // AstSliceSel  -> Create a temporary variable and refer the variable
 
+// Compare AstNode* to get deterministic ordering when showing messages.
+struct AstNodeComparator {
+    bool operator()(const AstNode* ap, const AstNode* bp) const {
+        const int lineComp = ap->fileline()->operatorCompare(*bp->fileline());
+        if (lineComp != 0) return lineComp < 0;
+        return ap < bp;
+    }
+};
+
 class UnpackRef {
     // m_nodep is called in this context (AstNodeStmt, AstCell, AstNodeFTask, or AstAlways)
     AstNode* m_contextp;
@@ -277,6 +284,9 @@ public:
     }
     bool lvalue() const { return m_lvalue; }
     bool ftask() const { return m_ftask; }
+    bool operator<(const UnpackRef& other) const {
+        return AstNodeComparator()(m_nodep, other.m_nodep);
+    }
 };
 
 class UnpackRefMap {
@@ -289,7 +299,7 @@ public:
             return a.nodep() == b.nodep();
         }
     };
-    typedef std::map<AstVar*, vl_unordered_set<UnpackRef, Hash, Compare> > MapType;
+    typedef std::map<AstVar*, std::set<UnpackRef>, AstNodeComparator> MapType;
     typedef MapType::iterator MapIt;
     typedef MapType::value_type::second_type::iterator SetIt;
 
@@ -336,30 +346,14 @@ public:
     MapIt end() { return m_map.end(); }
 };
 
-// Compare AstNode* to get deterministic ordering when showing messages.
-struct AstNodeComparator {
-    bool operator()(const AstNode* ap, const AstNode* bp) const {
-        const FileLine* afp = ap->fileline();
-        const FileLine* bfp = bp->fileline();
-        if (afp->firstLineno() != bfp->firstLineno())
-            return afp->firstLineno() < bfp->firstLineno();
-        if (afp->firstColumn() != bfp->firstColumn())
-            return afp->firstColumn() < bfp->firstColumn();
-        // Don't comapre its filename because it is expensive.
-        // Line number and column is practically enough.
-        // The comparison of raw pointer here is just in case.
-        // The comparison of this pointer may differ on different run,
-        // but this is just warning message order. (mostly for CI)
-        // Verilated result is same anyway.
-        return ap < bp;
-    }
-};
-
 // Found nodes for SplitPackedVarVisitor
 struct RefsInModule {
-    std::set<AstVar*> m_vars;
+    typedef std::set<AstVar*, AstNodeComparator> VarSet;
-    std::set<AstVarRef*> m_refs;
+    typedef std::set<AstVarRef*, AstNodeComparator> VarRefSet;
-    std::set<AstSel*> m_sels;
+    typedef std::set<AstSel*, AstNodeComparator> SelSet;
+    VarSet m_vars;
+    VarRefSet m_refs;
+    SelSet m_sels;
 
 public:
     void add(AstVar* nodep) { m_vars.insert(nodep); }
@@ -381,12 +375,10 @@ public:
         v.iterate(nodep);
     }
     void visit(AstNVisitor* visitor) {
-        for (std::set<AstVar*>::iterator it = m_vars.begin(), it_end = m_vars.end(); it != it_end;
+        for (VarSet::iterator it = m_vars.begin(), it_end = m_vars.end(); it != it_end; ++it) {
-             ++it) {
             visitor->iterate(*it);
         }
-        for (std::set<AstSel*>::iterator it = m_sels.begin(), it_end = m_sels.end(); it != it_end;
+        for (SelSet::iterator it = m_sels.begin(), it_end = m_sels.end(); it != it_end; ++it) {
-             ++it) {
             // If m_refs includes VarRef from ArraySel, remove it
             // because the VarRef would not be visited in SplitPackedVarVisitor::visit(AstSel*).
             if (AstVarRef* refp = VN_CAST((*it)->fromp(), VarRef)) {
@@ -399,8 +391,7 @@ public:
             UASSERT_OBJ(reinterpret_cast<uintptr_t>((*it)->op1p()) != 1, *it, "stale");
             visitor->iterate(*it);
         }
-        for (std::set<AstVarRef*>::iterator it = m_refs.begin(), it_end = m_refs.end();
+        for (VarRefSet::iterator it = m_refs.begin(), it_end = m_refs.end(); it != it_end; ++it) {
-             it != it_end; ++it) {
             UASSERT_OBJ(reinterpret_cast<uintptr_t>((*it)->op1p()) != 1, *it, "stale");
             visitor->iterate(*it);
         }
@@ -894,15 +885,16 @@ class PackedVarRef {
     AstBasicDType* m_basicp;  // Cache the ptr since varp->dtypep()->basicp() is expensive
     bool m_dedupDone;
     static void dedupRefs(std::vector<PackedVarRefEntry>& refs) {
-        vl_unordered_map<AstNode*, size_t> nodes;
+        // Use raw pointer to dedup
+        typedef std::map<AstNode*, size_t, AstNodeComparator> NodeIndices;
+        NodeIndices nodes;
         for (size_t i = 0; i < refs.size(); ++i) {
             nodes.insert(std::make_pair(refs[i].nodep(), i));
         }
         std::vector<PackedVarRefEntry> vect;
         vect.reserve(nodes.size());
-        for (vl_unordered_map<AstNode*, size_t>::const_iterator it = nodes.begin(),
+        for (NodeIndices::const_iterator it = nodes.begin(), it_end = nodes.end(); it != it_end;
-                                                                it_end = nodes.end();
+             ++it) {
-             it != it_end; ++it) {
             vect.push_back(refs[it->second]);
         }
         refs.swap(vect);
@@ -983,11 +975,12 @@ public:
 };
 
 class SplitPackedVarVisitor : public AstNVisitor, public SplitVarImpl {
+    typedef std::map<AstVar*, PackedVarRef, AstNodeComparator> PackedVarRefMap;
     AstNetlist* m_netp;
     AstNodeModule* m_modp;  // Current module (just for log)
     int m_numSplit;  // Total number of split variables
     // key:variable to be split. value:location where the variable is referenced.
-    vl_unordered_map<AstVar*, PackedVarRef> m_refs;
+    PackedVarRefMap m_refs;
     virtual void visit(AstNodeModule* nodep) VL_OVERRIDE {
         UASSERT_OBJ(m_modp == NULL, m_modp, "Nested module declaration");
         if (!VN_IS(nodep, Module)) {
@@ -1017,7 +1010,7 @@ class SplitPackedVarVisitor : public AstNVisitor, public SplitVarImpl {
     virtual void visit(AstVarRef* nodep) VL_OVERRIDE {
         AstVar* varp = nodep->varp();
         visit(varp);
-        vl_unordered_map<AstVar*, PackedVarRef>::iterator refit = m_refs.find(varp);
+        PackedVarRefMap::iterator refit = m_refs.find(varp);
         if (refit == m_refs.end()) return;  // variable without split_var metacomment
         UASSERT_OBJ(varp->attrSplitVar(), varp, "split_var attribute must be attached");
         UASSERT_OBJ(!nodep->packagep(), nodep,
@@ -1036,7 +1029,7 @@ class SplitPackedVarVisitor : public AstNVisitor, public SplitVarImpl {
         }
 
         AstVar* varp = vrefp->varp();
-        vl_unordered_map<AstVar*, PackedVarRef>::iterator refit = m_refs.find(varp);
+        PackedVarRefMap::iterator refit = m_refs.find(varp);
         if (refit == m_refs.end()) {
             iterateChildren(nodep);
             return;  // Variable without split_var metacomment
@@ -1198,9 +1191,8 @@ class SplitPackedVarVisitor : public AstNVisitor, public SplitVarImpl {
     }
     // Do the actual splitting operation
     void split() {
-        for (vl_unordered_map<AstVar*, PackedVarRef>::iterator it = m_refs.begin(),
+        for (PackedVarRefMap::iterator it = m_refs.begin(), it_end = m_refs.end(); it != it_end;
-                                                               it_end = m_refs.end();
+             ++it) {
-             it != it_end; ++it) {
             it->second.dedup();
             AstVar* varp = it->first;
             UINFO(3, "In module " << m_modp->name() << " var " << varp->prettyNameQ()

test_regress/t/t_split_var_1_bad.out

@@ -1,48 +1,65 @@
-                                              : ... In instance t
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:7:13: 'should_show_warning_global0' has split_var metacomment, but will not be split because it is not declared in a module.
-                                              : ... In instance t
-                                              : ... In instance t
-                                              : ... In instance t
-                                              : ... In instance t.i_sub0
-                                              : ... In instance t.i_sub1
-                                              : ... In instance t.i_sub1
-                                              : ... In instance t.i_sub3
-                                              : ... In instance t.i_sub3
-                                             : ... In instance t
-                                           : ... In instance t
-                                   : ... In instance t.i_sub2
-                   ... Use "/* verilator lint_off SPLITVAR */" and lint_on around source to disable this message.
-      |                                            ^~~~~~~~~~
-      |                                          ^~~~~~~~
-      |                                  ^~
-      |                                 ^
-      |                               ^
-      |                               ^~~~
-      |                             ^
-      |                ^
-      |                ^~~~~~~~~~~~~~~~~~~~~~~~
-      |                ^~~~~~~~~~~~~~~~~~~~~~~~
-      |              ^~~~~~~~~~~~~
-      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
-      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
-      |           ^~~~~~~~~~~~~~~~~~~
-      |           ^~~~~~~~~~~~~~~~~~~~
-      |           ^~~~~~~~~~~~~~~~~~~~
-      |         ^~~~~~~~~~~~~~~~~~~~
     7 | logic [7:0] should_show_warning_global0 /*verilator split_var*/ ;
+      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
+                   ... Use "/* verilator lint_off SPLITVAR */" and lint_on around source to disable this message.
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:8:13: 'should_show_warning_global1' has split_var metacomment, but will not be split because it is not declared in a module.
     8 | logic [7:0] should_show_warning_global1 [1:0] /*verilator split_var*/ ;
+      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:11:16: 'should_show_warning_ifs0' has split_var metacomment, but will not be split because it is not declared in a module.
    11 |    logic [7:0] should_show_warning_ifs0 /*verilator split_var*/ ;
+      |                ^~~~~~~~~~~~~~~~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:12:16: 'should_show_warning_ifs1' has split_var metacomment, but will not be split because it is not declared in a module.
    12 |    logic [7:0] should_show_warning_ifs1 [1:0] /*verilator split_var*/ ;
-   17 |    real should_show_warning0                  /*verilator split_var*/ ;
+      |                ^~~~~~~~~~~~~~~~~~~~~~~~
-   18 |    string should_show_warning1                /*verilator split_var*/ ;
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:38:14: 'cannot_split1' has split_var metacomment but will not be split because it is accessed from another module via a dot.
-   19 |    wire   should_show_warning2                /*verilator split_var*/ ;
-   30 |    function int bad_func(inout logic [3:0] inout_port /*verilator split_var*/ ,
-   31 |                          ref logic [7:0] ref_port /*verilator split_var*/ );
    38 |       i_sub0.cannot_split1[0] = 0;
+      |              ^~~~~~~~~~~~~
+%Warning-SELRANGE: t/t_split_var_1_bad.v:83:33: Selection index out of range: 13 outside 12:10
+                                              : ... In instance t.i_sub3
+   83 |    assign outwires[12] = inwires[13];   
+      |                                 ^
+%Warning-WIDTH: t/t_split_var_1_bad.v:39:31: Operator ASSIGN expects 8 bits on the Assign RHS, but Assign RHS's FUNCREF 'bad_func' generates 32 bits.
+                                           : ... In instance t
    39 |       i_sub0.cannot_split1[1] = bad_func(addr, rd_data0);
-   51 |       rd_data = cannot_split0[addr];
+      |                               ^
-   57 |    genvar cannot_split_genvar /*verilator split_var*/ ;
+%Error: t/t_split_var_1_bad.v:72:16: Illegal assignment of constant to unpacked array
-   60 |       rd_data = cannot_split[addr];
+                                   : ... In instance t.i_sub2
    72 |    assign b = a[0];   
+      |                ^
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:51:31: 'cannot_split0' has split_var metacomment but will not be split because index cannot be determined statically.
+                                              : ... In instance t.i_sub0
+   51 |       rd_data = cannot_split0[addr];
+      |                               ^~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:83:34: 'inwires' has split_var metacomment but will not be split because index is out of range.
+                                              : ... In instance t.i_sub3
    83 |    assign outwires[12] = inwires[13];   
-   83 |    assign outwires[12] = inwires[13];   
+      |                                  ^~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:17:9: 'should_show_warning0' has split_var metacomment but will not be split because it is not an aggregate type of bit nor logic
+                                             : ... In instance t
+   17 |    real should_show_warning0                  /*verilator split_var*/ ;
+      |         ^~~~~~~~~~~~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:18:11: 'should_show_warning1' has split_var metacomment but will not be split because it is not an aggregate type of bit nor logic
+                                              : ... In instance t
+   18 |    string should_show_warning1                /*verilator split_var*/ ;
+      |           ^~~~~~~~~~~~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:19:11: 'should_show_warning2' has split_var metacomment but will not be split because its bitwidth is 1
+                                              : ... In instance t
+   19 |    wire   should_show_warning2                /*verilator split_var*/ ;
+      |           ^~~~~~~~~~~~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:30:44: 'inout_port' has split_var metacomment but will not be split because it is an inout port
+                                              : ... In instance t
+   30 |    function int bad_func(inout logic [3:0] inout_port /*verilator split_var*/ ,
+      |                                            ^~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:31:42: 'ref_port' has split_var metacomment but will not be split because it is a ref argument
+                                              : ... In instance t
+   31 |                          ref logic [7:0] ref_port /*verilator split_var*/ );
+      |                                          ^~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:57:11: 'cannot_split_genvar' has split_var metacomment but will not be split because it is not an aggregate type of bit nor logic
+                                              : ... In instance t.i_sub1
+   57 |    genvar cannot_split_genvar /*verilator split_var*/ ;
+      |           ^~~~~~~~~~~~~~~~~~~
+%Warning-SPLITVAR: t/t_split_var_1_bad.v:60:29: 'cannot_split' has split_var metacomment but will not be split because its bit range cannot be determined statically.
+                                              : ... In instance t.i_sub1
+   60 |       rd_data = cannot_split[addr];
+      |                             ^
 %Error: Exiting due to

test_regress/t/t_split_var_1_bad.pl

@@ -13,12 +13,8 @@ scenarios(linter => 1);
 lint(
     fails => 1,
     verilator_flags2 => ['--stats'],
-    # When issue-2407 resolved, decomment and update golden file
+    expect_filename => $Self->{golden_filename},
-    #expect_filename => $Self->{golden_filename},
 );
 
-# When issue-2407 resolved, remove
-files_identical_sorted("$Self->{obj_dir}/vlt_compile.log", $Self->{golden_filename}, 1);
-
 ok(1);
 1;