diff --git a/src/V3FileLine.h b/src/V3FileLine.h index ef195d246..4cc568886 100644 --- a/src/V3FileLine.h +++ b/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; diff --git a/src/V3SplitVar.cpp b/src/V3SplitVar.cpp index 5cc78823a..f0096ba37 100644 --- a/src/V3SplitVar.cpp +++ b/src/V3SplitVar.cpp @@ -122,8 +122,6 @@ #include #include #include -#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 > MapType; + typedef std::map, 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 m_vars; - std::set m_refs; - std::set m_sels; + typedef std::set VarSet; + typedef std::set VarRefSet; + typedef std::set 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::iterator it = m_vars.begin(), it_end = m_vars.end(); it != it_end; - ++it) { + for (VarSet::iterator it = m_vars.begin(), it_end = m_vars.end(); it != it_end; ++it) { visitor->iterate(*it); } - for (std::set::iterator it = m_sels.begin(), it_end = m_sels.end(); it != it_end; - ++it) { + for (SelSet::iterator it = m_sels.begin(), it_end = m_sels.end(); it != it_end; ++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((*it)->op1p()) != 1, *it, "stale"); visitor->iterate(*it); } - for (std::set::iterator it = m_refs.begin(), it_end = m_refs.end(); - it != it_end; ++it) { + for (VarRefSet::iterator it = m_refs.begin(), it_end = m_refs.end(); it != it_end; ++it) { UASSERT_OBJ(reinterpret_cast((*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& refs) { - vl_unordered_map nodes; + // Use raw pointer to dedup + typedef std::map NodeIndices; + NodeIndices nodes; for (size_t i = 0; i < refs.size(); ++i) { nodes.insert(std::make_pair(refs[i].nodep(), i)); } std::vector vect; vect.reserve(nodes.size()); - for (vl_unordered_map::const_iterator it = nodes.begin(), - it_end = nodes.end(); - it != it_end; ++it) { + for (NodeIndices::const_iterator it = nodes.begin(), it_end = nodes.end(); 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 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 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::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::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::iterator it = m_refs.begin(), - it_end = m_refs.end(); - it != it_end; ++it) { + for (PackedVarRefMap::iterator it = m_refs.begin(), it_end = m_refs.end(); it != it_end; + ++it) { it->second.dedup(); AstVar* varp = it->first; UINFO(3, "In module " << m_modp->name() << " var " << varp->prettyNameQ() diff --git a/test_regress/t/t_split_var_1_bad.out b/test_regress/t/t_split_var_1_bad.out index 96b602356..b51f38b0b 100644 --- a/test_regress/t/t_split_var_1_bad.out +++ b/test_regress/t/t_split_var_1_bad.out @@ -1,48 +1,65 @@ - : ... In instance t - : ... 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. - | ^~~~~~~~~~ - | ^~~~~~~~ - | ^~ - | ^ - | ^ - | ^~~~ - | ^ - | ^ - | ^~~~~~~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~~ - | ^~~~~~~~~~~~~~~~~~~~ +%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. 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*/ ; - 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*/ ); + | ^~~~~~~~~~~~~~~~~~~~~~~~ +%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. 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*/ ; - 60 | rd_data = cannot_split[addr]; + | ^ +%Error: t/t_split_var_1_bad.v:72:16: Illegal assignment of constant to unpacked array + : ... 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 diff --git a/test_regress/t/t_split_var_1_bad.pl b/test_regress/t/t_split_var_1_bad.pl index bbfbd2b0c..cb986145a 100755 --- a/test_regress/t/t_split_var_1_bad.pl +++ b/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;