diff --git a/src/V3Order.cpp b/src/V3Order.cpp index 54cb7fba3..c6b67e7e6 100644 --- a/src/V3Order.cpp +++ b/src/V3Order.cpp @@ -107,7 +107,8 @@ AstCFunc* V3Order::order(AstNetlist* netlistp, // bool slow, // const ExternalDomainsProvider& externalDomains) { // Build the OrderGraph - const std::unique_ptr graph = buildOrderGraph(netlistp, logic, trigToSen); + const std::unique_ptr graph + = buildOrderGraph(netlistp, logic, trigToSen, parallel); // Order it orderOrderGraph(*graph, tag); // Assign sensitivity domains to combinational logic diff --git a/src/V3OrderGraph.h b/src/V3OrderGraph.h index b40f3b5f5..7359680ea 100644 --- a/src/V3OrderGraph.h +++ b/src/V3OrderGraph.h @@ -70,6 +70,8 @@ #include "V3Ast.h" #include "V3Graph.h" +#include + class OrderLogicVertex; class OrderVarVertex; @@ -139,9 +141,21 @@ public: class OrderLogicVertex final : public OrderEitherVertex { VL_RTTI_IMPL(OrderLogicVertex, OrderEitherVertex) + +public: + // Variable access record + struct VarAccess final { + AstVarScope* m_vscp; // The variable accessed + VAccess m_access; // The kind of access, as in the AST + }; + +private: AstNode* const m_nodep; // The logic this vertex represents AstScope* const m_scopep; // Scope the logic is under AstSenTree* const m_hybridp; // Additional sensitivities for hybrid combinational logic + // Every variable accessed by this logic, in order of first access, at most one record per + // variable. Only populated for multi-threaded ordering. + std::vector m_varAccesses; public: // CONSTRUCTOR @@ -163,6 +177,10 @@ public: AstNode* nodep() const VL_MT_STABLE { return m_nodep; } AstScope* scopep() const VL_MT_STABLE { return m_scopep; } AstSenTree* hybridp() const { return m_hybridp; } + const std::vector& varAccesses() const { return m_varAccesses; } + void addVarAccess(AstVarScope* vscp, VAccess access) { + m_varAccesses.push_back({vscp, access}); + } // LCOV_EXCL_START // Debug code string name() const override VL_MT_STABLE { diff --git a/src/V3OrderGraphBuilder.cpp b/src/V3OrderGraphBuilder.cpp index 48a65d7a7..0ecb89e35 100644 --- a/src/V3OrderGraphBuilder.cpp +++ b/src/V3OrderGraphBuilder.cpp @@ -76,20 +76,24 @@ public: class OrderGraphBuilder final : public VNVisitor { // TYPES enum VarUsage : uint8_t { VU_CON = 0x1, VU_GEN = 0x2 }; + enum VarAccess : uint8_t { VA_READ = 0x1, VA_WRITE = 0x2 }; using VarVertexType = OrderUser::VarVertexType; // NODE STATE // AstVarScope::user1 -> OrderUser instance for variable (via m_orderUser) // AstVarScope::user2 -> VarUsage within logic blocks // AstVarScope::user3 -> bool: Hybrid sensitivity + // AstVarScope::user4 -> VarAccess within logic blocks const VNUser1InUse user1InUse; const VNUser2InUse user2InUse; const VNUser3InUse user3InUse; + const VNUser4InUse user4InUse; AstUser1Allocator m_orderUser; // STATE OrderGraph* const m_graphp = new OrderGraph; // The ordering graph built by this visitor OrderLogicVertex* m_logicVxp = nullptr; // Current logic block being analyzed + std::vector m_accessedVscps; // Variables accessed by the current logic block // Map from Trigger reference AstSenItem to the original AstSenTree const V3Order::TrigToSenMap& m_trigToSen; @@ -106,13 +110,15 @@ class OrderGraphBuilder final : public VNVisitor { bool m_inPost = false; // Underneath AstAlwaysPost std::function m_readTriggersCombLogic; V3Sched::util::VarScopeSet m_forceReadEdgeIgnores; + const bool m_parallel; // Ordering for multi-threaded execution (record variable accesses) // METHODS void iterateLogic(AstNode* nodep) { UASSERT_OBJ(!m_logicVxp, nodep, "Should not nest"); - // Reset VarUsage + // Reset VarUsage and VarAccess AstNode::user2ClearTree(); + AstNode::user4ClearTree(); m_forceReadEdgeIgnores.clear(); if (!m_inClocked) V3Sched::util::collectForceReadEdgeIgnores(nodep, m_forceReadEdgeIgnores); @@ -120,6 +126,17 @@ class OrderGraphBuilder final : public VNVisitor { m_logicVxp = new OrderLogicVertex{m_graphp, m_scopep, m_domainp, m_hybridp, nodep}; // Gather variable dependencies based on usage iterateChildren(nodep); + if (m_parallel) { + // Emit one access record for each variable this logic block accessed + for (AstVarScope* const vscp : m_accessedVscps) { + const int recorded = vscp->user4(); + const VAccess access = recorded == (VA_READ | VA_WRITE) ? VAccess::READWRITE + : recorded == VA_WRITE ? VAccess::WRITE + : VAccess::READ; + m_logicVxp->addVarAccess(vscp, access); + } + m_accessedVscps.clear(); + } // Finished with this logic m_logicVxp = nullptr; m_forceReadEdgeIgnores.clear(); @@ -184,6 +201,16 @@ class OrderGraphBuilder final : public VNVisitor { // Variable reference in logic. Add data dependency. + // Record the raw access for the multi-threaded data hazard fixer + if (m_parallel) { + uint8_t recorded = 0; + if (nodep->access().isWriteOrRW()) recorded |= VA_WRITE; + if (nodep->access().isReadOrRW()) recorded |= VA_READ; + UASSERT_OBJ(recorded, nodep, "Unknown variable access type"); + // Accumulate access type, record the variable on first access only + if (!varscp->user4Or(recorded)) m_accessedVscps.push_back(varscp); + } + // Check whether this variable was already generated/consumed in the same logic. We // don't want to add extra edges if the logic has many usages of the same variable, // so only proceed on first encounter. @@ -357,8 +384,9 @@ class OrderGraphBuilder final : public VNVisitor { // CONSTRUCTOR OrderGraphBuilder(AstNetlist* /*nodep*/, const std::vector& coll, - const V3Order::TrigToSenMap& trigToSen) - : m_trigToSen{trigToSen} { + const V3Order::TrigToSenMap& trigToSen, bool parallel) + : m_trigToSen{trigToSen} + , m_parallel{parallel} { // Build the graph for (const V3Sched::LogicByScope* const lbsp : coll) { for (const auto& pair : *lbsp) { @@ -375,14 +403,17 @@ public: // this visitor does change the tree (removes some nodes related to DPI export trigger). static std::unique_ptr apply(AstNetlist* nodep, const std::vector& coll, - const V3Order::TrigToSenMap& trigToSen) { - return std::unique_ptr{OrderGraphBuilder{nodep, coll, trigToSen}.m_graphp}; + const V3Order::TrigToSenMap& trigToSen, + bool parallel) { + return std::unique_ptr{ + OrderGraphBuilder{nodep, coll, trigToSen, parallel}.m_graphp}; } }; std::unique_ptr V3Order::buildOrderGraph(AstNetlist* netlistp, // const std::vector& coll, // - const V3Order::TrigToSenMap& trigToSen) { - return OrderGraphBuilder::apply(netlistp, coll, trigToSen); + const V3Order::TrigToSenMap& trigToSen, // + bool parallel) { + return OrderGraphBuilder::apply(netlistp, coll, trigToSen, parallel); } diff --git a/src/V3OrderInternal.h b/src/V3OrderInternal.h index 789a6e6d8..facaab652 100644 --- a/src/V3OrderInternal.h +++ b/src/V3OrderInternal.h @@ -42,7 +42,8 @@ namespace V3Order { std::unique_ptr buildOrderGraph(AstNetlist* netlistp, // const std::vector& coll, // - const TrigToSenMap& trigToSen); + const TrigToSenMap& trigToSen, // + bool parallel); void orderOrderGraph(OrderGraph& graph, const std::string& tag); diff --git a/src/V3OrderMTaskContraction.cpp b/src/V3OrderMTaskContraction.cpp index af49acd72..382f9f921 100644 --- a/src/V3OrderMTaskContraction.cpp +++ b/src/V3OrderMTaskContraction.cpp @@ -527,8 +527,9 @@ class Contraction final { m_mTaskGraph.mergeMTasks(recipientp, donorp); VL_DANGLING(donorp); - // Confirm we haven't botched the CP updates. - m_mTaskGraph.validate(); + // Confirm we haven't botched the CP updates. This is a whole graph walk after every single + // merge, so it is quadratic in the size of the graph, hence only under '--debug 9'. + if (VL_UNLIKELY(debug() >= 9)) m_mTaskGraph.validate(); // Add the EdgeMCs of the merged MTask addEdgeMCs(recipientp); diff --git a/src/V3OrderMTaskFixHazards.cpp b/src/V3OrderMTaskFixHazards.cpp index 17563ea95..298529317 100644 --- a/src/V3OrderMTaskFixHazards.cpp +++ b/src/V3OrderMTaskFixHazards.cpp @@ -12,6 +12,86 @@ // SPDX-FileCopyrightText: 2003-2026 Wilson Snyder // SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 // +// The fine-grained graph from V3Order may contain data hazards which are not a problem for +// serial mode, but which would be a problem in parallel mode. There are two classes: +// unordered pairs of writes, and unordered write-read pairs. This transform adds edges here +// until no such unordered pair remains. +// +// ABOUT UNORDERED WRITE-WRITE PAIRS +// +// The V3Order dependency graph treats these as unordered events: +// +// a) sig[15:8] = stuff; +// ... +// b) sig[7:0] = other_stuff; +// +// They are writes to disjoint bits of the same signal. They can run in +// either order, in serial mode, and the result will be the same. +// +// The resulting C code for each of this isn't a pure write, it's actually an R-M-W +// sequence: +// +// a) sig = (sig & 0xff) | (0xff00 & (stuff << 8)); +// ... +// b) sig = (sig & 0xff00) | (0xff & other_stuff); +// +// In serial mode, order doesn't matter as these run serially. In parallel mode, +// they must be serialized to avoid a race. +// +// This pass does not check if each write would involve an R-M-W, it just assumes that +// it does. If this routine ever causes a drastic increase in critical path, it could be +// optimized to make a better prediction (with all the risk that word implies!) about +// whether a given write is likely to turn into an R-M-W. +// +// ABOUT UNORDERED WRITE-READ PAIRS +// +// If we don't put unordered write-read pairs into some order at Verilation time, we risk +// a runtime race. +// +// These arise because the OrderGraph deliberately does not model every access. A read of +// a variable that is in the reading block's own hybrid sensitivity list gets no edge, as +// does a read ignored due to a force/release, or an access to a variable marked +// 'ignoreSchedWrite' and friends. For serial mode that is fine: whatever order the logic +// ends up in, it runs one block at a time. In parallel mode two such blocks can run +// concurrently, and if one of them writes what the other reads, that is an observable +// data race. +// +// HOW TO FIX THEM +// +// An arbitrary ordering is prescribed by adding edges between MTasks. The new edges +// must not create a cycle, and should be added in a way that increases critical paths +// as little as possible. +// +// Every MTask is given a unique sequence number, in a topological order of the graph, so +// that for every edge 'from -> to' we have seq(from) < seq(to). Adding a new edge that +// runs in increasing sequence order therefore cannot create a cycle, and the property is +// maintained by induction as more edges are added. +// +// With that in hand, for each variable we take its accessors in sequence order, chain the +// writers together, and bracket each reader between the writers either side of it. +// Readers need no ordering with respect to one another. +// +// The sequence numbers are assigned by a topological sort that emits the ready MTask with +// the smallest forward critical path first. As the forward critical path of an MTask is at +// least that of each of its predecessors, the smallest among the ready MTasks is also the +// smallest among all not yet emitted ones, so MTasks come out in globally non-decreasing +// forward critical path order. Chaining them in that order is what tends to grow the +// critical path least: the longest path through a chain starts at the head of its first +// MTask, so putting those with the shortest path to them first keeps that sum down. +// +// The critical paths are also what makes finding the edges that are actually needed cheap. +// An edge is only added between a pair that is not ordered already, and a pair can be +// ruled ordered, or not, in constant time whenever their critical paths are inconsistent +// with a path between them, which avoids searching the graph for most pairs. +// +// SystemC variables are handled similarly, except that all SystemC variables are treated as +// a single entity. Reasoning: writing a systemC var actually turns into a call to a var.write() +// method, which under the hood is accessing some data structure that's shared by many SC vars. +// It's not thread safe. +// +// DPI calls are serialized similarly (they are assumed not thread safe), unless directed by +// options. +// //************************************************************************* #include "V3PchAstNoMT.h" // VL_MT_DISABLED_CODE_UNIT @@ -19,12 +99,10 @@ #include "V3Control.h" #include "V3Global.h" #include "V3Graph.h" -#include "V3GraphStream.h" #include "V3OrderGraph.h" #include "V3OrderMTaskGraph.h" #include -#include #include #include @@ -72,140 +150,140 @@ public: // FixDataHazards class FixDataHazards final { - // - // Fix data hazards in the MTask graph. - // - // The fine-grained graph from V3Order may contain data hazards which are - // not a problem for serial mode, but which would be a problem in parallel - // mode. - // - // There are basically two classes: unordered pairs of writes, and - // unordered write-read pairs. We fix both here, with a combination of - // MTask-merges and new edges to ensure no such unordered pairs remain. - // - // ABOUT UNORDERED WRITE-WRITE PAIRS - // - // The V3Order dependency graph treats these as unordered events: - // - // a) sig[15:8] = stuff; - // ... - // b) sig[7:0] = other_stuff; - // - // Seems OK right? They are writes to disjoint bits of the same - // signal. They can run in either order, in serial mode, and the result - // will be the same. - // - // The resulting C code for each of this isn't a pure write, it's - // actually an R-M-W sequence: - // - // a) sig = (sig & 0xff) | (0xff00 & (stuff << 8)); - // ... - // b) sig = (sig & 0xff00) | (0xff & other_stuff); - // - // In serial mode, order doesn't matter so long as these run serially. - // In parallel mode, we must serialize these RMW's to avoid a race. - // - // We don't actually check here if each write would involve an R-M-W, we - // just assume that it would. If this routine ever causes a drastic - // increase in critical path, it could be optimized to make a better - // prediction (with all the risk that word implies!) about whether a - // given write is likely to turn into an R-M-W. - // - // ABOUT UNORDERED WRITE-READ PAIRS - // - // If we don't put unordered write-read pairs into some order at Verilation - // time, we risk a runtime race. - // - // How do such unordered writer/reader pairs happen? Here's a partial list - // of scenarios: - // - // Case 1: Circular logic - // - // If the design has circular logic, V3Order has by now generated some - // dependency cycles, and also cut some of the edges to make it - // acyclic. - // - // For serial mode, that was fine. We can break logic circles at an - // arbitrary point. At runtime, we'll repeat the _eval() until no - // changes are detected, which papers over the discarded dependency. - // - // For parallel mode, this situation can lead to unordered reads and - // writes of the same variable, causing a data race. For example if the - // original code is this: - // - // assign b = b | a << 2; - // assign out = b; - // - // ... there's originally a dependency edge which records that 'b' - // depends on the first assign. V3Order may cut this edge, making the - // statements unordered. In serial mode that's fine, they can run in - // either order. In parallel mode it's a reader/writer race. - // - // Case 2: Race Condition in Verilog Sources - // - // If the input has races, eg. blocking assignments in always blocks - // that share variables, the graph at this point will contain unordered - // writes and reads (or unordered write-write pairs) reflecting that. - // TYPES - // Sort LogicMTask objects into deterministic order by calling id() - // which is a unique and stable serial number. - struct MTaskIdLessThan final { - bool operator()(const LogicMTask* lhsp, const LogicMTask* rhsp) const { - return *lhsp < *rhsp; - } + + // Access to one variable, with the MTask performing it + struct Access final { + const AstVarScope* m_vscp; // The variable accessed, its index is in 'user1' + LogicMTask* m_mtaskp; // The accessing MTask + VAccess m_access; // The kind of access }; - using TasksByRank = std::map>; + + // NODE STATE + // AstVarScope::user1 -> int: Variable index for stable sorting + const VNUser1InUse m_user1InUse; // MEMBERS OrderMTaskGraph& m_mTaskGraph; // The Mtask graph + std::vector m_scratch; // Scratch MTask list, reused to avoid reallocation // METHODS - void findAdjacentTasks(const OrderVarStdVertex* varVtxp, TasksByRank& tasksByRank) { - // Find all writer tasks for this variable, group by rank. - for (const V3GraphEdge& edge : varVtxp->inEdges()) { - if (const auto* const logicVtxp = edge.fromp()->cast()) { - LogicMTask* const writerMtaskp = static_cast(logicVtxp->userp()); - tasksByRank[writerMtaskp->rank()].insert(writerMtaskp); + // Assign each MTask a unique sequence number, held in 'user()', such that for every edge + // 'from -> to' we have seq(from) < seq(to). See the note at top of file on why this matters. + // This is a topological sort using Kahn's algorithm with MTasks enumerated in a globally + // non-decreasing critical path order. + void assignSequenceNumbers() { + struct MTaskCmp final { + bool operator()(const LogicMTask* ap, const LogicMTask* bp) const { + // Order by critical path + const uint64_t aCp = ap->cpExclusive(); + const uint64_t bCp = bp->cpExclusive(); + if (aCp != bCp) return aCp < bCp; + // Break ties by stable id + return *ap < *bp; + } + }; + // Set of ready vertices. Initialized to the entry MTask. + std::set ready{m_mTaskGraph.entryp()}; + // 'user' also used to count remaining dependencies of each MTask. Initialize it. + for (V3GraphVertex& vtx : m_mTaskGraph.vertices()) { + vtx.user(static_cast(vtx.inEdges().size())); + } + // Next sequence number to assign to each MTask. + uint32_t seq = 0; + // Process ready vertices in critical path order. + while (!ready.empty()) { + // Pick up and detach the ready MTask with the smallest critical path. + const auto it = ready.begin(); + LogicMTask* const mtaskp = *it; + ready.erase(it); + // Assign the next sequence number to the MTask + mtaskp->user(++seq); + // Decrement edge count of successors and add to ready set if no dependencies left + for (V3GraphEdge& edge : mtaskp->outEdges()) { + LogicMTask* const top = static_cast(edge.top()); + const uint32_t nDeps = top->user() - 1; + top->user(nDeps); + if (!nDeps) ready.insert(top); } } - // Note: Find all reader tasks for this variable, group by rank. - // There was "broken" code here to find readers, but fixing it to - // work properly harmed performance on some tests, see issue #3360. + UASSERT(seq == m_mTaskGraph.vertices().size(), "MTask graph is cyclic"); } - void mergeSameRankTasks(const TasksByRank& tasksByRank) { - LogicMTask* lastRecipientp = nullptr; - for (const auto& pair : tasksByRank) { - // Find the largest node at this rank, merge into it. (If we - // happen to find a huge node, this saves time in the merge - // versus merging into an arbitrary node.) - LogicMTask* recipientp = nullptr; - for (LogicMTask* const mtaskp : pair.second) { - if (!recipientp || (recipientp->cost() < mtaskp->cost())) recipientp = mtaskp; - } - UASSERT_OBJ(!lastRecipientp || (lastRecipientp->rank() < recipientp->rank()), - recipientp, "Merging must be on lower rank"); - - for (LogicMTask* const donorp : pair.second) { - // Merge donor into recipient. - if (donorp == recipientp) continue; - // Fix up the map, so donor's OLVs map to recipientp. Must do this while the donor - // still holds them. - for (const OrderMoveVertex& vtx : donorp->vertexList()) { - vtx.logicp()->userp(recipientp); + // Gather every variable access, resolved to the MTask performing it. + std::vector gatherAccesses() { + std::vector accesses; + int nVars = 0; + for (V3GraphVertex& vtx : m_mTaskGraph.vertices()) { + LogicMTask& mtask = static_cast(vtx); + for (const OrderMoveVertex& mVtx : mtask.vertexList()) { + const OrderLogicVertex* const lVtxp = mVtx.logicp(); + if (!lVtxp) continue; + for (const OrderLogicVertex::VarAccess& acc : lVtxp->varAccesses()) { + AstVarScope* const vscp = acc.m_vscp; + if (!vscp->user1()) vscp->user1(++nVars); + accesses.push_back({vscp, &mtask, acc.m_access}); } - // Merge donorp into recipientp, which also deletes donorp - m_mTaskGraph.mergeMTasks(recipientp, donorp); - VL_DANGLING(donorp); } - - if (lastRecipientp && !lastRecipientp->hasEdgeTo(recipientp)) { - m_mTaskGraph.addEdge(lastRecipientp, recipientp); - } - lastRecipientp = recipientp; } + return accesses; + } + + // Add an edge ordering 'fromp' before 'top', unless they are already ordered + void addEdgeIfNeeded(LogicMTask* fromp, LogicMTask* top) { + // Nothing to order within a single MTask + if (fromp == top) return; + UASSERT_OBJ(fromp->user() < top->user(), fromp, + "Edge must run in increasing sequence order"); + // Already directly ordered. This is an O(1) set lookup, and catches the common case of + // a dependency the OrderGraph already provided. + if (fromp->hasEdgeTo(top)) return; + // Otherwise check if already ordered + if (m_mTaskGraph.pathExists(fromp, top, nullptr)) return; + // Unordered. Add an edge between them. + m_mTaskGraph.addEdge(fromp, top); + } + + // Add the edges required to make the accesses of one variable race free. 'beginp'/'endp' + // delimit the accesses of a single variable, in sequence number order. + void serializeVariable(const Access* beginp, const Access* endp) { + // Nothing to order if at most one MTask accesses this variable. Each MTask appears at + // most once in the range, see the assertion below, so this is just the range size. + if (endp - beginp < 2) return; + + std::vector& pendingReaders = m_scratch; + pendingReaders.clear(); + LogicMTask* prevWriterp = nullptr; + for (const Access* accp = beginp; accp != endp; ++accp) { + // The OrderLogicVertices record one access per variable, and at this point each MTask + // holds exactly one logic block, so an MTask cannot appear twice here. + UASSERT_OBJ(accp == beginp || accp[-1].m_mtaskp != accp->m_mtaskp, accp->m_mtaskp, + "Multiple accesses of a variable in an MTask"); + LogicMTask* const mtaskp = accp->m_mtaskp; + if (accp->m_access.isWriteOrRW()) { + // Reads since the previous write must complete before this write + for (LogicMTask* const readerp : pendingReaders) addEdgeIfNeeded(readerp, mtaskp); + // Consecutive writes must be ordered. If a read came between them, the edges to + // and from that read imply it already. + if (prevWriterp && pendingReaders.empty()) addEdgeIfNeeded(prevWriterp, mtaskp); + pendingReaders.clear(); + prevWriterp = mtaskp; + } else { + // This read must happen after the preceding write + if (prevWriterp) addEdgeIfNeeded(prevWriterp, mtaskp); + pendingReaders.push_back(mtaskp); + } + } + } + + // Serialize the given MTasks, in sequence number order + void serializeMTasks(std::vector& mtaskps) { + std::sort(mtaskps.begin(), mtaskps.end(), [](const LogicMTask* ap, const LogicMTask* bp) { + return ap->user() < bp->user(); + }); + mtaskps.erase(std::unique(mtaskps.begin(), mtaskps.end()), mtaskps.end()); + for (size_t i = 1; i < mtaskps.size(); ++i) addEdgeIfNeeded(mtaskps[i - 1], mtaskps[i]); } bool hasDpiHazard(LogicMTask* mtaskp) { @@ -226,115 +304,47 @@ class FixDataHazards final { // CONSTRUCTOR FixDataHazards(OrderMTaskGraph& mTaskGraph) : m_mTaskGraph{mTaskGraph} { - // Rank the graph. DGS is faster than V3GraphAlg's recursive rank, and also allows us to - // set up the OrderLogicVertex -> LogicMTask map at the same time. + // Give the MTasks a total order consistent with their dependencies + assignSequenceNumbers(); + + // Gather variable accesses made by each MTask + std::vector accesses = gatherAccesses(); + // Sort by variable (to group by variable), then by sequence number of the accessing MTask + std::sort(accesses.begin(), accesses.end(), [](const Access& a, const Access& b) { + if (a.m_vscp != b.m_vscp) return a.m_vscp->user1() < b.m_vscp->user1(); + return a.m_mtaskp->user() < b.m_mtaskp->user(); + }); + + // Serialize the accesses of each variable + for (size_t i = 0; i < accesses.size();) { + size_t end = i + 1; + while (end < accesses.size() && accesses[end].m_vscp == accesses[i].m_vscp) ++end; + serializeVariable(accesses.data() + i, accesses.data() + end); + i = end; + } + + // Serialize all writes to SystemC vars. Note the reads of an individual SC var are already + // ordered against its writes by the per variable pass above, it is only the writes between + // different SC vars that need this extra serialization. Only top level ports are SC vars, + // so this should not hurt performance too much. { - GraphStreamUnordered serialize{&m_mTaskGraph}; - while (LogicMTask* const mtaskp - = const_cast(static_cast(serialize.nextp()))) { - // Compute and assign rank - uint32_t rank = 0; - for (V3GraphEdge& edge : mtaskp->inEdges()) { - rank = std::max(edge.fromp()->rank() + 1, rank); - } - mtaskp->rank(rank); - - // Set up the OrderLogicVertex -> LogicMTask map - // Entry and exit MTasks have no MTaskMoveVertices under them, so move on - if (mtaskp->vertexList().empty()) continue; - // Otherwise there should be only one OrderMoveVertex in each MTask at this stage - const OrderMoveVertex::List& vertexList = mtaskp->vertexList(); - UASSERT_OBJ(vertexList.hasSingleElement(), mtaskp, "Multiple OrderMoveVertex"); - const OrderMoveVertex* const mVtxp = vertexList.frontp(); - // Set up mapping back to the MTask from the OrderLogicVertex - if (OrderLogicVertex* const lvtxp = mVtxp->logicp()) lvtxp->userp(mtaskp); + m_scratch.clear(); + for (const Access& access : accesses) { + if (!access.m_access.isWriteOrRW()) continue; + if (!access.m_vscp->varp()->isSc()) continue; + m_scratch.push_back(access.m_mtaskp); } + serializeMTasks(m_scratch); } - // Gather all variables. SystemC vars will be handled slightly specially, so keep separate. - const OrderGraph& orderGraph = m_mTaskGraph.moveGraph().orderGraph(); - std::vector regularVars; - std::vector systemCVars; - for (const V3GraphVertex& vtx : orderGraph.vertices()) { - // Only consider OrderVarStdVertex which reflects - // an actual lvalue assignment; the others do not. - if (const OrderVarStdVertex* const vvtxp = vtx.cast()) { - if (vvtxp->vscp()->varp()->isSc()) { - systemCVars.push_back(vvtxp); - } else { - regularVars.push_back(vvtxp); - } - } - } - - // For each OrderVarVertex, look at its writer and reader MTasks. - // - // If there's a set of writers and readers at the same rank, we - // know these are unordered with respect to one another, so merge - // those MTasks all together. - // - // At this point, we have at most one merged mtask per rank (for a - // given OVV.) Create edges across these remaining MTasks to ensure - // they run in serial order (going along with the existing ranks.) - // - // NOTE: all graph mutations below go through OrderMTaskGraph (adding an edge, or merging - // two MTasks), so the CP's stored in the LogicMTasks are kept up to date throughout. - for (const OrderVarStdVertex* const varVtxp : regularVars) { - // Build a set of MTasks, per rank, which access this var. - // Within a rank, sort by MTaskID to avoid nondeterminism. - TasksByRank tasksByRank; - - // Find all reader and writer tasks for this variable, add to - // tasksByRank. - findAdjacentTasks(varVtxp, tasksByRank); - - // Merge all writer and reader tasks from same rank together. - // - // NOTE: Strictly speaking, we don't need to merge all the - // readers together. That may lead to extra serialization. The - // least amount of ordering we could impose here would be to - // merge all writers at a given rank together; then make edges - // from the merged writer node to each reader node at the same - // rank; and then from each reader node to the merged writer at - // the next rank. - // - // Whereas, merging all readers and writers at the same rank - // together is "the simplest thing that could possibly work" - // and it seems to. It also creates fairly few edges. We don't - // want to create tons of edges here, doing so is not nice to - // the main edge contraction pass. - mergeSameRankTasks(tasksByRank); - } - - // Handle SystemC vars just a little differently. Instead of - // treating each var as an independent entity, and serializing - // writes to that one var, we treat ALL systemC vars as a single - // entity and serialize writes (and, conservatively, reads) across - // all of them. - // - // Reasoning: writing a systemC var actually turns into a call to a - // var.write() method, which under the hood is accessing some data - // structure that's shared by many SC vars. It's not thread safe. - // - // Hopefully we only have a few SC vars -- top level ports, probably. - { - TasksByRank tasksByRank; - for (const OrderVarStdVertex* const varVtxp : systemCVars) { - findAdjacentTasks(varVtxp, tasksByRank); - } - mergeSameRankTasks(tasksByRank); - } - - // Handle nodes containing DPI calls, we want to serialize those - // by default unless user gave '--threads-dpi none'. - // Same basic strategy as above to serialize access to SC vars. + // Serialize DPI calls unless user gave '--threads-dpi none'. if (!v3Global.opt.threadsDpiPure() || !v3Global.opt.threadsDpiUnpure()) { - TasksByRank tasksByRank; + m_scratch.clear(); for (V3GraphVertex& vtx : m_mTaskGraph.vertices()) { LogicMTask& mtask = static_cast(vtx); - if (hasDpiHazard(&mtask)) tasksByRank[mtask.rank()].insert(&mtask); + if (hasDpiHazard(&mtask)) m_scratch.push_back(&mtask); } - mergeSameRankTasks(tasksByRank); + serializeMTasks(m_scratch); } } diff --git a/src/V3OrderMTaskGraph.cpp b/src/V3OrderMTaskGraph.cpp index 50d3a175c..b2cca479a 100644 --- a/src/V3OrderMTaskGraph.cpp +++ b/src/V3OrderMTaskGraph.cpp @@ -261,6 +261,78 @@ void OrderMTaskGraph::mergeMTasks(LogicMTask* recipientp, LogicMTask* donorp) { VL_DO_DANGLING(donorp->unlinkDelete(this), donorp); } +void OrderMTaskGraph::removeTransitiveEdges() { + // Removing a transitive edge cannot change any critical path, so none need updating here. + // Only the edge heaps and the dependent sets need maintaining. + for (V3GraphVertex& vtx : vertices()) { + for (V3GraphEdge* const graphEdgep : vtx.outEdges().unlinkable()) { + MTaskEdge* const edgep = static_cast(graphEdgep); + LogicMTask* const fromp = edgep->fromMTaskp(); + LogicMTask* const top = edgep->toMTaskp(); + // If the MTasks are also connected by some other path, then this is a transitive edge + if (!pathExists(fromp, top, edgep)) continue; + // Maintain the additional data structures of the OrderMTaskGraph + fromp->removeDependent(top); + fromp->removeRelativeEdge(edgep); + top->removeRelativeEdge(edgep); + VL_DO_DANGLING(edgep->unlinkDelete(), edgep); + } + } + // Confirm the above left the maintained state consistent + validate(); +} + +void OrderMTaskGraph::removeEmptyMTasks() { + // This transform preserves the critical paths as it connects every predecessor of the + // removed MTask to every successor, and the removed MTask itself has zero cost. + for (V3GraphVertex* const vtxp : vertices().unlinkable()) { + LogicMTask* const mtaskp = static_cast(vtxp); + + // Keep the entry and exit vertices. + if (mtaskp == m_entryp || mtaskp == m_exitp) continue; + + // Keep any MTask that holds logic + bool empty = true; + for (const OrderMoveVertex& mVtx : mtaskp->vertexList()) { + if (mVtx.logicp()) { + empty = false; + break; + } + } + if (!empty) continue; + + // The MTask holding no logic should have zero cost + UASSERT_OBJ(!mtaskp->cost(), mtaskp, "MTask holding no logic should have 0 cost"); + + // Connect each predecessor directly to each successor. + for (V3GraphEdge& inEdge : mtaskp->inEdges()) { + LogicMTask* const fromp = static_cast(inEdge).fromMTaskp(); + for (V3GraphEdge& outEdge : mtaskp->outEdges()) { + LogicMTask* const top = static_cast(outEdge).toMTaskp(); + if (!fromp->hasEdgeTo(top)) addEdge(fromp, top); + } + } + + // Remove incoming edges of 'mtaskp' + while (MTaskEdge* const edgep = static_cast(mtaskp->inEdges().frontp())) { + LogicMTask* const relativep = edgep->fromMTaskp(); + relativep->removeDependent(mtaskp); + relativep->removeRelativeEdge(edgep); + VL_DO_DANGLING(edgep->unlinkDelete(), edgep); + } + // Remove outgoing edges of 'mtaskp' + while (MTaskEdge* const edgep = static_cast(mtaskp->outEdges().frontp())) { + LogicMTask* const relativep = edgep->toMTaskp(); + relativep->removeRelativeEdge(edgep); + VL_DO_DANGLING(edgep->unlinkDelete(), edgep); + } + // Delete the empty MTask + VL_DO_DANGLING(mtaskp->unlinkDelete(this), mtaskp); + } + // Confirm the above left the maintained state consistent + validate(); +} + // Check the critical paths in the given direction, and the critical paths cached in the edge heaps // in the opposite direction, against those implied by the edges. Note this deliberately iterates // the edge lists, rather than consulting the edge heaps, so the heaps are validated, not trusted. diff --git a/src/V3OrderMTaskGraph.h b/src/V3OrderMTaskGraph.h index 39dd7e7b1..9995cfd06 100644 --- a/src/V3OrderMTaskGraph.h +++ b/src/V3OrderMTaskGraph.h @@ -349,9 +349,18 @@ public: // released any auxiliary data it attached to them via their user pointer. void mergeMTasks(LogicMTask* recipientp, LogicMTask* donorp); - // Do an EXPENSIVE check that the maintained critical paths, including the ones cached in the - // edge heaps, match those implied by the current edges of the graph, and that the graph itself - // is consistent. Does nothing unless 'slowAsserts', so it is safe to call unconditionally. + // Remove all transitive edges. (This deliberately hides V3Graph::removeTransitiveEdges, + // which would leave the auxiliary data structures stale.) + // cppcheck-suppress duplInheritedMember + void removeTransitiveEdges(); + + // Remove all MTasks holding no logic (except for entry and exit, which are kept even if + // empty), connecting their predecessors directly to their successors. + void removeEmptyMTasks(); + + // Do an expensive check that the maintained critical paths, including the ones cached in the + // edge heaps, match those implied by the current edges of the graph, and that auxiliary data + // structures are consistent. void validate() const; // STATIC METHODS diff --git a/src/V3OrderParallel.cpp b/src/V3OrderParallel.cpp index a68ceed97..3bc200cd8 100644 --- a/src/V3OrderParallel.cpp +++ b/src/V3OrderParallel.cpp @@ -21,7 +21,9 @@ #include "V3PchAstNoMT.h" // VL_MT_DISABLED_CODE_UNIT #include "V3Ast.h" +#include "V3AstUserAllocator.h" #include "V3Control.h" +#include "V3Error.h" #include "V3ExecGraph.h" #include "V3Graph.h" #include "V3GraphStream.h" @@ -29,11 +31,86 @@ #include "V3OrderInternal.h" #include "V3OrderMTaskGraph.h" +#include #include #include +#include VL_DEFINE_DEBUG_FUNCTIONS; +//###################################################################### +// Data hazard checker + +// Reports read-write and write-write pairs on the same variable that +// are not ordered in the MTask graph. +static void checkDataHazards(OrderMTaskGraph& mTaskGraph) { + // Expensive, so only with '--debug-partition' + if (!mTaskGraph.slowAsserts()) return; + + // Order MTasks by their stable ids, so the report is deterministic + struct MTaskIdLessThan final { + bool operator()(const LogicMTask* ap, const LogicMTask* bp) const { return *ap < *bp; } + }; + struct VarInfo final { + bool m_seen = false; // Variable already appended to 'vscps' + // How each MTask accesses the variable, merged over the logic within that MTask + std::map m_byMTask; + }; + + // AstVarScope::user1 -> VarInfo instance for the variable (via 'varInfos') + const VNUser1InUse user1InUse; + AstUser1Allocator varInfos; + + // The variables accessed (in enumerated order, for stability). + std::vector vscps; + + // Gather how each MTask accesses each variable + for (V3GraphVertex& vtx : mTaskGraph.vertices()) { + LogicMTask& mtask = static_cast(vtx); + for (const OrderMoveVertex& mVtx : mtask.vertexList()) { + const OrderLogicVertex* const lVtxp = mVtx.logicp(); + if (!lVtxp) continue; // A variable vertex, which performs no access itself + for (const OrderLogicVertex::VarAccess& acc : lVtxp->varAccesses()) { + AstVarScope* const vscp = acc.m_vscp; + VarInfo& varInfo = varInfos(vscp); + if (!varInfo.m_seen) { + varInfo.m_seen = true; + vscps.push_back(vscp); + } + const auto pair = varInfo.m_byMTask.emplace(&mtask, acc.m_access); + // Merge the access kinds if this MTask already accessed this variable + if (!pair.second && pair.first->second != acc.m_access) { + pair.first->second = VAccess::READWRITE; + } + } + } + } + + // Report every unordered pair of accessors where at least one side writes + AstVarScope* firstHazardp = nullptr; // First variable with a hazard, for the error below + for (AstVarScope* const vscp : vscps) { + const auto& byMTask = varInfos(vscp).m_byMTask; + for (auto aIt = byMTask.begin(); aIt != byMTask.end(); ++aIt) { + for (auto bIt = std::next(aIt); bIt != byMTask.end(); ++bIt) { + // Concurrent reads are not a hazard + if (aIt->second.isReadOnly() && bIt->second.isReadOnly()) continue; + LogicMTask* const ap = aIt->first; + LogicMTask* const bp = bIt->first; + if (mTaskGraph.pathExists(ap, bp, nullptr)) continue; + if (mTaskGraph.pathExists(bp, ap, nullptr)) continue; + // LCOV_EXCL_START + if (!firstHazardp) firstHazardp = vscp; + UINFO(0, "Data hazard: " << vscp->name() << " " << aIt->second.ascii() << " by mt" + << ap->id() << ", " << bIt->second.ascii() << " by mt" + << bp->id() << " (unordered)"); + // LCOV_EXCL_STOP + } + } + } + // Fail if any hazards were found + if (firstHazardp) firstHazardp->v3fatalSrc("Data hazards found"); // LCOV_EXCL_BR_LINE +} + //###################################################################### // Partitioner implementation @@ -48,7 +125,7 @@ static std::unique_ptr partition(OrderMoveGraph& moveGraph) { std::unique_ptr mTaskGraphp = OrderMTaskGraph::build(moveGraph); mTaskGraphp->hashGraphDebug("initial MTask graph"); - // Merge nodes that could present data hazards + // Add edges to eliminate data hazards OrderMTaskGraph::fixDataHazards(*mTaskGraphp); mTaskGraphp->hashGraphDebug("MTask graph after fixDataHazards()"); @@ -71,36 +148,24 @@ static std::unique_ptr partition(OrderMoveGraph& moveGraph) { mTaskGraphp->hashGraphDebug("MTask graph after contract()"); } - // Note the graph is only mutated by generic V3Graph algorithms from here on. These neither - // maintain the critical paths of the MTasks, nor create MTaskEdges when rerouting, so the - // critical paths are stale below, and the graph must not be handed back to OrderMTaskGraph. + // Remove MTasks that have no logic in them, rerouting the edges + mTaskGraphp->removeEmptyMTasks(); + mTaskGraphp->hashGraphDebug("MTask graph after removeEmptyMTasks()"); + + // Note this is OrderMTaskGraph::removeTransitiveEdges, which maintains graph consistency mTaskGraphp->removeTransitiveEdges(); mTaskGraphp->hashGraphDebug("MTask graph after removeTransitiveEdges()"); - // Remove MTasks that have no logic in it, rerouting the edges. Set user to indicate the - // mtask on every underlying OrderMoveVertex. Clear vertex lists (used later). + // Check for data hazards the partitioning left unordered + checkDataHazards(*mTaskGraphp); + + // Set OrderMoveVertex::userp to indicate the mtask it is part of. moveGraph.userClearVertices(); for (V3GraphVertex* const vtxp : mTaskGraphp->vertices().unlinkable()) { LogicMTask* const mtaskp = vtxp->as(); OrderMoveVertex::List& vertexList = mtaskp->vertexList(); - // Check if MTask is empty - bool empty = true; - for (const OrderMoveVertex& mVtx : vertexList) { - if (mVtx.logicp()) { - empty = false; - break; - } - } - // If empty remove it now - if (empty) { - mtaskp->rerouteEdges(mTaskGraphp.get()); - VL_DO_DANGLING(mtaskp->unlinkDelete(mTaskGraphp.get()), mtaskp); - continue; - } - // Annotate the underlying OrderMoveVertex vertices and unlink them while (OrderMoveVertex* const mVtxp = vertexList.unlinkFront()) mVtxp->userp(mtaskp); } - mTaskGraphp->removeRedundantEdgesSum(&V3GraphEdge::followAlwaysTrue); // Return the resulting MTask graph return mTaskGraphp; @@ -194,6 +259,14 @@ AstNodeStmt* V3Order::createParallel(OrderMoveGraph& moveGraph, const std::strin const LogicMTask* const cMTaskp = vtxp->as(); LogicMTask* const mTaskp = const_cast(cMTaskp); + // The entry and exit vertices only anchor the graph, they hold no logic and + // must not become ExecMTasks. + if (mTaskp == mTaskGraphp->entryp() || mTaskp == mTaskGraphp->exitp()) { + UASSERT_OBJ(mTaskp->vertexList().empty(), mTaskp, + "Entry and exit vertices should have no logic"); + continue; + } + // Add initially ready vertices within this MTask to the serializer as seeds, // and unlink them from the vertex list in the MTask as we go. (The serializer // uses the list links in the vertex, so must unlink it here.) @@ -238,6 +311,8 @@ AstNodeStmt* V3Order::createParallel(OrderMoveGraph& moveGraph, const std::strin for (const V3GraphEdge& edge : mTaskp->inEdges()) { const V3GraphVertex* fromVxp = edge.fromp(); const LogicMTask* const fromp = fromVxp->as(); + // Skip the entry vertex, which has no ExecMTask + if (fromp == mTaskGraphp->entryp()) continue; new V3GraphEdge{depGraphp, logicMTaskToExecMTask.at(fromp), execMTaskp, 1}; } } diff --git a/test_regress/t/t_sched_hybrid_hazard.py b/test_regress/t/t_sched_hybrid_hazard.py index 6b088e9bd..76563844a 100755 --- a/test_regress/t/t_sched_hybrid_hazard.py +++ b/test_regress/t/t_sched_hybrid_hazard.py @@ -17,6 +17,6 @@ test.enable_tsan() test.compile(verilator_flags2=['--binary', '-fno-dfg', '--no-threads-coarsen'], threads=2) -test.execute(fails='any') # Now failing, fix pending +test.execute() test.passes()