From 192445097ac1e5413c7e1203aa4dc2eb7b1b4a07 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sun, 1 Mar 2026 11:59:29 -0500 Subject: [PATCH] Internals: Fix non-determinism in V3Delayed, V3SplitVar, V3Task (#7120 partial) (#7165) --- src/CMakeLists.txt | 3 ++- src/V3Container.h | 65 ++++++++++++++++++++++++++++++++++++++++++++++ src/V3Delayed.cpp | 2 +- src/V3Options.cpp | 20 +++++--------- src/V3PchAstMT.h | 1 + src/V3PchAstNoMT.h | 1 + src/V3SplitVar.cpp | 44 ++++++++++++++++++++++++------- src/V3Task.cpp | 13 +++++----- src/V3Task.h | 9 ++++--- 9 files changed, 123 insertions(+), 35 deletions(-) create mode 100644 src/V3Container.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7c0612dee..92bd02219 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -65,8 +65,9 @@ set(HEADERS V3Clock.h V3Combine.h V3Common.h - V3Control.h V3Const.h + V3Container.h + V3Control.h V3Coverage.h V3CoverageJoin.h V3Dead.h diff --git a/src/V3Container.h b/src/V3Container.h new file mode 100644 index 000000000..7644cbea6 --- /dev/null +++ b/src/V3Container.h @@ -0,0 +1,65 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// DESCRIPTION: Verilator: Generic container types +// +// Code available from: https://verilator.org +// +//************************************************************************* +// +// 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-FileCopyrightText: 2003-2026 Wilson Snyder +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 +// +//************************************************************************* + +#ifndef VERILATOR_V3CONTAINER_H_ +#define VERILATOR_V3CONTAINER_H_ + +#include "config_build.h" +#include "verilatedos.h" + +#include +#include +#include + +//============================================================================ + +// Similar to std::set, but ordered based on call order to emplace. Used +// when insertion order is desired (e.g. std::vector), but duplicates need removal. +// Keys may not be modified. (If needed in future, m_set could contain vector positions.) +template +class VInsertionSet final { + std::vector m_keys; // Elements by insertion order + std::unordered_set m_keySet; // Elements by key +public: + // METHODS + bool insert(const T_Key& key) { + // Returns if did insertion (second pair argument of traditional emplace) + const auto itFoundPair = m_keySet.insert(key); + if (itFoundPair.second) m_keys.push_back(key); + return itFoundPair.second; + } + void clear() { + m_keys.clear(); + m_keySet.clear(); + } + + // ACCESSORS + bool empty() const { return m_keys.empty(); } + bool exists(const T_Key& key) const { return m_keySet.find(key) != m_keySet.end(); } + + // ITERATORS + using const_iterator = typename std::vector::const_iterator; + const_iterator begin() const { return m_keys.begin(); } + const_iterator end() const { return m_keys.end(); } +}; + +//============================================================================ +// VInsertionMap: Not currently needed; prototype code exists, just ask. +// Similar to std::map, but ordered based on call order to emplace. Used +// when insertion order is desired (e.g. std::vector), but duplicates need removal. +// Values may be modified. + +#endif // Guard diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index 209218b8e..49aeb4705 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -263,7 +263,7 @@ class DelayedVisitor final : public VNVisitor { AstUser3Allocator> m_writeRefs; // STATE - across all visitors - std::set m_timingDomains; // Timing resume domains + VInsertionSet m_timingDomains; // Timing resume domains // STATE - for current visit position (use VL_RESTORER) AstActive* m_activep = nullptr; // Current activate diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 28c7d75b8..7018bc296 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -18,6 +18,7 @@ #include "V3Options.h" +#include "V3Container.h" #include "V3Error.h" #include "V3File.h" #include "V3Global.h" @@ -70,30 +71,26 @@ public: std::list m_lineArgs; // List of command line argument encountered // List of arguments encounterd, and a bool in needed for rerunning --dump-inputs std::list, bool>> m_allArgs; - std::list m_incDirUsers; // Include directories (ordered) - std::set m_incDirUserSet; // Include directories (for removing duplicates) + VInsertionSet m_incDirUsers; // Include directories (ordered) std::list m_incDirFallbacks; // Include directories (ordered) std::set m_incDirFallbackSet; // Include directories (for removing duplicates) std::map m_langExts; // Language extension map - std::list m_libExtVs; // Library extensions (ordered) - std::set m_libExtVSet; // Library extensions (for removing duplicates) + VInsertionSet m_libExtVs; // Library extensions (ordered) DirMap m_dirMap; // Directory listing // ACCESSOR METHODS void addIncDirUser(const string& incdir) { const string& dir = V3Os::filenameCleanup(incdir); - const auto itFoundPair = m_incDirUserSet.insert(dir); - if (itFoundPair.second) { + const bool inserted = m_incDirUsers.insert(dir); + if (inserted) { // cppcheck-suppress stlFindInsert // cppcheck 1.90 bug - m_incDirUsers.push_back(dir); m_incDirFallbacks.remove(dir); // User has priority over Fallback m_incDirFallbackSet.erase(dir); // User has priority over Fallback } } void addIncDirFallback(const string& incdir) { const string& dir = V3Os::filenameCleanup(incdir); - if (m_incDirUserSet.find(dir) - == m_incDirUserSet.end()) { // User has priority over Fallback + if (!m_incDirUsers.exists(dir)) { // User has priority over Fallback const auto itFoundPair = m_incDirFallbackSet.insert(dir); if (itFoundPair.second) m_incDirFallbacks.push_back(dir); } @@ -106,10 +103,7 @@ public: m_langExts[addext] = lc; } - void addLibExtV(const string& libext) { - const auto itFoundPair = m_libExtVSet.insert(libext); - if (itFoundPair.second) m_libExtVs.push_back(libext); - } + void addLibExtV(const string& libext) { m_libExtVs.insert(libext); } V3OptionsImp() = default; ~V3OptionsImp() = default; }; diff --git a/src/V3PchAstMT.h b/src/V3PchAstMT.h index 34f862088..751afd41d 100644 --- a/src/V3PchAstMT.h +++ b/src/V3PchAstMT.h @@ -26,6 +26,7 @@ #include "V3Ast.h" #include "V3Broken.h" +#include "V3Container.h" #include "V3Error.h" #include "V3FileLine.h" #include "V3FunctionTraits.h" diff --git a/src/V3PchAstNoMT.h b/src/V3PchAstNoMT.h index e778dab5d..d2ddc1d6e 100644 --- a/src/V3PchAstNoMT.h +++ b/src/V3PchAstNoMT.h @@ -28,6 +28,7 @@ #include "V3Ast.h" #include "V3Broken.h" +#include "V3Container.h" #include "V3Error.h" #include "V3FileLine.h" #include "V3FunctionTraits.h" diff --git a/src/V3SplitVar.cpp b/src/V3SplitVar.cpp index 62c386293..95bd05954 100644 --- a/src/V3SplitVar.cpp +++ b/src/V3SplitVar.cpp @@ -226,15 +226,37 @@ static void warnNoSplit(const AstVar* varp, const AstNode* wherep, const char* r // Split Unpacked Variables // Replacement policy: // AstArraySel -> Just replace with the AstVarRef for the split variable -// AstVarRef -> Create a temporary variable and refer the variable -// AstSliceSel -> Create a temporary variable and refer the variable +// AstVarRef -> Create a temporary variable and refer to the variable +// AstSliceSel -> Create a temporary variable and refer to the variable -// Compare AstNode* to get deterministic ordering when showing messages. +// Track order-of-encounter for nodes, so we are stable, versus comparing node pointers +// (fileline may be the same across multiple nodes, so is insufficient) +class SplitNodeOrder final { + // NODE STATE + // AstNode::user4() -> uint64_t. Order the node is in the tree + const VNUser4InUse m_user4InUse; + +public: + static uint64_t nextId() { + static uint64_t s_sequence = 0; + return ++s_sequence; + } + static uint64_t nodeOrder(const AstNode* const nodep) { + AstNode* const ncnodep = const_cast(nodep); + const uint64_t id = ncnodep->user4(); + if (VL_LIKELY(id)) return id; + ncnodep->user4(nextId()); + return ncnodep->user4(); + } +}; + +// Compare AstNode* to get deterministic ordering struct AstNodeComparator final { bool operator()(const AstNode* ap, const AstNode* bp) const { + // First consider lines, as makes messages to user more obvious const int lineComp = ap->fileline()->operatorCompare(*bp->fileline()); if (lineComp != 0) return lineComp < 0; - return ap < bp; + return SplitNodeOrder::nodeOrder(ap) < SplitNodeOrder::nodeOrder(bp); } }; @@ -247,6 +269,7 @@ class UnpackRef final { const int m_lsb; // for SliceSel const VAccess m_access; const bool m_ftask; // true if the reference is in function/task. false if in module. + public: UnpackRef(AstNode* stmtp, AstVarRef* nodep, bool ftask) : m_contextp{stmtp} @@ -587,12 +610,12 @@ class SplitUnpackedVarVisitor final : public VNVisitor, public SplitVarImpl { iterateChildren(nodep); } } - AstVarRef* createTempVar(AstNode* context, AstNode* nodep, AstUnpackArrayDType* dtypep, + AstVarRef* createTempVar(AstNode* contextp, AstNode* nodep, AstUnpackArrayDType* dtypep, const std::string& name_prefix, std::vector& vars, int start_idx, bool lvalue, bool /*ftask*/) { FileLine* const fl = nodep->fileline(); const std::string name = m_tempNames.get(nodep) + "__" + name_prefix; - AstNodeAssign* const assignp = VN_CAST(context, NodeAssign); + AstNodeAssign* const assignp = VN_CAST(contextp, NodeAssign); if (assignp) { // "always_comb a = b;" to "always_comb begin a = b; end" so that local // variable can be added. @@ -604,7 +627,7 @@ class SplitUnpackedVarVisitor final : public VNVisitor, public SplitVarImpl { << " is created lsb:" << dtypep->lo() << " msb:" << dtypep->hi()); // Use AstAssign if true, otherwise AstAssignW const bool use_simple_assign - = (context && VN_IS(context, NodeFTaskRef)) || (assignp && VN_IS(assignp, Assign)); + = (contextp && VN_IS(contextp, NodeFTaskRef)) || (assignp && VN_IS(assignp, Assign)); for (int i = 0; i < dtypep->elementsConst(); ++i) { AstNodeExpr* lhsp @@ -618,11 +641,11 @@ class SplitUnpackedVarVisitor final : public VNVisitor, public SplitVarImpl { AstAssign* const ap = new AstAssign{fl, lhsp, rhsp}; if (lvalue) { // If varp is LHS, this assignment must appear after the original - // assignment(context). - context->addNextHere(ap); + // assignment(contextp). + contextp->addNextHere(ap); } else { // If varp is RHS, this assignment comes just before the original assignment - context->addHereThisAsNext(ap); + contextp->addHereThisAsNext(ap); } UASSERT_OBJ(!m_contextp, m_contextp, "must be null"); setContextAndIterate(ap, refp); @@ -1364,6 +1387,7 @@ const char* SplitVarImpl::cannotSplitPackedVarReason(const AstVar* varp) { void V3SplitVar::splitVariable(AstNetlist* nodep) { UINFO(2, __FUNCTION__ << ":"); + SplitNodeOrder order; SplitVarRefs refs; { const SplitUnpackedVarVisitor visitor{nodep}; diff --git a/src/V3Task.cpp b/src/V3Task.cpp index a3fa624a8..a5da23eeb 100644 --- a/src/V3Task.cpp +++ b/src/V3Task.cpp @@ -1856,7 +1856,7 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp, if (!makeChanges) return tconnects; // Connect missing ones - std::set argWrap; // Which ports are defaulted, forcing arg wrapper creation + VInsertionSet argWrap; // Which ports are defaulted; need arg wrapper creation for (int i = 0; i < tpinnum; ++i) { AstVar* const portp = tconnects[i].first; if (!tconnects[i].second || !tconnects[i].second->exprp()) { @@ -1882,7 +1882,7 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp, if (statep) { portp->pinNum(i + 1); // Make sure correct, will use to build name UINFO(9, "taskConnects arg wrapper needed " << portp->valuep()); - argWrap.emplace(portp); + argWrap.insert(portp); } else { // statep = nullptr, called too late or otherwise to handle args // Problem otherwise is we might have a varref, task // call, or something else that only makes sense in the @@ -1956,7 +1956,8 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp, } void V3Task::taskConnectWrap(AstNodeFTaskRef* nodep, const V3TaskConnects& tconnects, - V3TaskConnectState* statep, const std::set& argWrap) { + V3TaskConnectState* statep, + const VInsertionSet& argWrap) { statep->setDidWrap(); // Make wrapper name such that is same iff same args are defaulted std::string newname = nodep->name() + "__Vtcwrap"; @@ -1973,7 +1974,7 @@ void V3Task::taskConnectWrap(AstNodeFTaskRef* nodep, const V3TaskConnects& tconn for (const auto& tconnect : tconnects) { const AstVar* const portp = tconnect.first; AstArg* const argp = tconnect.second; - if (argWrap.find(portp) != argWrap.end()) { // Removed arg + if (argWrap.exists(portp)) { // Removed arg statep->pushDeletep(argp->unlinkFrBack()); } } @@ -1985,7 +1986,7 @@ void V3Task::taskConnectWrap(AstNodeFTaskRef* nodep, const V3TaskConnects& tconn AstNodeFTask* V3Task::taskConnectWrapNew(AstNodeFTask* taskp, const string& newname, const V3TaskConnects& tconnects, - const std::set& argWrap) { + const VInsertionSet& argWrap) { std::map oldNewVars; // Old -> new var mappings AstNodeFTask* const newTaskp = taskp->cloneType(newname); @@ -2019,7 +2020,7 @@ AstNodeFTask* V3Task::taskConnectWrapNew(AstNodeFTask* taskp, const string& newn for (const auto& tconnect : tconnects) { AstVar* const portp = tconnect.first; AstVar* newPortp; - if (argWrap.find(portp) == argWrap.end()) { // Not removed arg + if (!argWrap.exists(portp)) { // Not removed arg newPortp = new AstVar{portp->fileline(), portp->varType(), portp->name(), portp}; newPortp->propagateWrapAttrFrom(portp); newPortp->funcLocal(true); diff --git a/src/V3Task.h b/src/V3Task.h index 378e87330..553af1f70 100644 --- a/src/V3Task.h +++ b/src/V3Task.h @@ -21,6 +21,7 @@ #include "verilatedos.h" #include "V3Ast.h" +#include "V3Container.h" #include "V3Error.h" #include @@ -58,10 +59,10 @@ public: bool makeChanges = true) VL_MT_DISABLED; static void taskConnectWrap(AstNodeFTaskRef* nodep, const V3TaskConnects& tconnects, V3TaskConnectState* statep, - const std::set& argWrap) VL_MT_DISABLED; - static AstNodeFTask* taskConnectWrapNew(AstNodeFTask* taskp, const string& newname, - const V3TaskConnects& tconnects, - const std::set& argWrap) VL_MT_DISABLED; + const VInsertionSet& argWrap) VL_MT_DISABLED; + static AstNodeFTask* + taskConnectWrapNew(AstNodeFTask* taskp, const string& newname, const V3TaskConnects& tconnects, + const VInsertionSet& argWrap) VL_MT_DISABLED; static string assignInternalToDpi(AstVar* portp, bool isPtr, const string& frSuffix, const string& toSuffix, const string& frPrefix = "") VL_MT_DISABLED;