Fix unordered data hazards in multi-threaded scheduling (#8133)

The OrderGraph used during V3Order step deliberately omits some variable
accesses from the dependency graph. E.g.: a read of a variable that is
in the reading block's own hybrid sensitivity list emits no edge, nor
does a read ignored due to a force/release, nor an access to a variable
marked 'ignoreSchedWrite' and friends. For serial mode that is fine, the
logic runs one block at a time. In parallel mode two such blocks can run
concurrently, and if one writes what the other reads, that is a data
race at runtime.

These accesses cannot be recovered from the graph edges. They are now
collected from the AST while the OrderGraph is built, and held by the
OrderLogicVertex performing them.

FixDataHazards is reworked around these access lists stored in
OrderLogicVertex, so it is now aware of all variable accesses the logic
makes, including those not encoded by the dependency graph edges. The
previous heuristic of fixing data hazards by merging same-rank MTasks is
removed. Additional edges are inserted instead to prescribe a fixed
ordering of conflicting MTasks. To insert edges without unduly
increasing the critical path, or introducing cycles, new edges are
added such that they preserve topological ordering, and they are
inserted between vertices sorted by critical path length. See algorithm
details in the code.

Also add a data hazard checker under '--debug-partition', reporting every
unordered accessor pair left in the final MTask graph.

This fixes the race demonstrated by t_sched_hybrid_hazard (#7913),
which is no longer expected to fail.

Under ThreadSanitizer over the vltmt tests: 17 failing before, 3 after,
with no regressions. The 3 remaining are different defects.

src/V3Order.cpp

@@ -107,7 +107,8 @@ AstCFunc* V3Order::order(AstNetlist* netlistp,  //
                          bool slow,  //
                          const ExternalDomainsProvider& externalDomains) {
     // Build the OrderGraph
-    const std::unique_ptr<OrderGraph> graph = buildOrderGraph(netlistp, logic, trigToSen);
+    const std::unique_ptr<OrderGraph> graph
+        = buildOrderGraph(netlistp, logic, trigToSen, parallel);
     // Order it
     orderOrderGraph(*graph, tag);
     // Assign sensitivity domains to combinational logic

src/V3OrderGraph.h

@@ -70,6 +70,8 @@
 #include "V3Ast.h"
 #include "V3Graph.h"
 
+#include <vector>
+
 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<VarAccess> 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<VarAccess>& 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 {

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<AstVarScope, OrderUser> 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<AstVarScope*> 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<bool(const AstVarScope*)> 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<V3Sched::LogicByScope*>& coll,
-                      const V3Order::TrigToSenMap& trigToSen)
+                      const V3Order::TrigToSenMap& trigToSen, bool parallel)
-        : m_trigToSen{trigToSen} {
+        : 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<OrderGraph> apply(AstNetlist* nodep,
                                              const std::vector<V3Sched::LogicByScope*>& coll,
-                                             const V3Order::TrigToSenMap& trigToSen) {
+                                             const V3Order::TrigToSenMap& trigToSen,
-        return std::unique_ptr<OrderGraph>{OrderGraphBuilder{nodep, coll, trigToSen}.m_graphp};
+                                             bool parallel) {
+        return std::unique_ptr<OrderGraph>{
+            OrderGraphBuilder{nodep, coll, trigToSen, parallel}.m_graphp};
     }
 };
 
 std::unique_ptr<OrderGraph>
 V3Order::buildOrderGraph(AstNetlist* netlistp,  //
                          const std::vector<V3Sched::LogicByScope*>& coll,  //
-                         const V3Order::TrigToSenMap& trigToSen) {
+                         const V3Order::TrigToSenMap& trigToSen,  //
-    return OrderGraphBuilder::apply(netlistp, coll, trigToSen);
+                         bool parallel) {
+    return OrderGraphBuilder::apply(netlistp, coll, trigToSen, parallel);
 }

src/V3OrderInternal.h

@@ -42,7 +42,8 @@ namespace V3Order {
 
 std::unique_ptr<OrderGraph> buildOrderGraph(AstNetlist* netlistp,  //
                                             const std::vector<V3Sched::LogicByScope*>& coll,  //
-                                            const TrigToSenMap& trigToSen);
+                                            const TrigToSenMap& trigToSen,  //
+                                            bool parallel);
 
 void orderOrderGraph(OrderGraph& graph, const std::string& tag);
 

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.
+        // Confirm we haven't botched the CP updates. This is a whole graph walk after every single
-        m_mTaskGraph.validate();
+        // 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);

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 <algorithm>
-#include <map>
 #include <set>
 #include <vector>
 
@@ -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.
+    // Access to one variable, with the MTask performing it
-    struct MTaskIdLessThan final {
+    struct Access final {
-        bool operator()(const LogicMTask* lhsp, const LogicMTask* rhsp) const {
+        const AstVarScope* m_vscp;  // The variable accessed, its index is in 'user1'
-            return *lhsp < *rhsp;
+        LogicMTask* m_mtaskp;  // The accessing MTask
-        }
+        VAccess m_access;  // The kind of access
     };
-    using TasksByRank = std::map<uint32_t /*rank*/, std::set<LogicMTask*, MTaskIdLessThan>>;
+
+    // NODE STATE
+    //  AstVarScope::user1    -> int: Variable index for stable sorting
+    const VNUser1InUse m_user1InUse;
 
     // MEMBERS
     OrderMTaskGraph& m_mTaskGraph;  // The Mtask graph
+    std::vector<LogicMTask*> m_scratch;  // Scratch MTask list, reused to avoid reallocation
 
     // METHODS
 
-    void findAdjacentTasks(const OrderVarStdVertex* varVtxp, TasksByRank& tasksByRank) {
+    // Assign each MTask a unique sequence number, held in 'user()', such that for every edge
-        // Find all writer tasks for this variable, group by rank.
+    // 'from -> to' we have seq(from) < seq(to). See the note at top of file on why this matters.
-        for (const V3GraphEdge& edge : varVtxp->inEdges()) {
+    // This is a topological sort using Kahn's algorithm with MTasks enumerated in a globally
-            if (const auto* const logicVtxp = edge.fromp()->cast<OrderLogicVertex>()) {
+    // non-decreasing critical path order.
-                LogicMTask* const writerMtaskp = static_cast<LogicMTask*>(logicVtxp->userp());
+    void assignSequenceNumbers() {
-                tasksByRank[writerMtaskp->rank()].insert(writerMtaskp);
+        struct MTaskCmp final {
+            bool operator()(const LogicMTask* ap, const LogicMTask* bp) const {
+                // Order by critical path
+                const uint64_t aCp = ap->cpExclusive<GraphWay::FORWARD>();
+                const uint64_t bCp = bp->cpExclusive<GraphWay::FORWARD>();
+                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<LogicMTask*, MTaskCmp> 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<uint32_t>(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<LogicMTask*>(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.
+        UASSERT(seq == m_mTaskGraph.vertices().size(), "MTask graph is cyclic");
-        // There was "broken" code here to find readers, but fixing it to
-        // work properly harmed performance on some tests, see issue #3360.
     }
 
-    void mergeSameRankTasks(const TasksByRank& tasksByRank) {
+    // Gather every variable access, resolved to the MTask performing it.
-        LogicMTask* lastRecipientp = nullptr;
+    std::vector<Access> gatherAccesses() {
-        for (const auto& pair : tasksByRank) {
+        std::vector<Access> accesses;
-            // Find the largest node at this rank, merge into it.  (If we
+        int nVars = 0;
-            // happen to find a huge node, this saves time in the merge
+        for (V3GraphVertex& vtx : m_mTaskGraph.vertices()) {
-            // versus merging into an arbitrary node.)
+            LogicMTask& mtask = static_cast<LogicMTask&>(vtx);
-            LogicMTask* recipientp = nullptr;
+            for (const OrderMoveVertex& mVtx : mtask.vertexList()) {
-            for (LogicMTask* const mtaskp : pair.second) {
+                const OrderLogicVertex* const lVtxp = mVtx.logicp();
-                if (!recipientp || (recipientp->cost() < mtaskp->cost())) recipientp = mtaskp;
+                if (!lVtxp) continue;
-            }
+                for (const OrderLogicVertex::VarAccess& acc : lVtxp->varAccesses()) {
-            UASSERT_OBJ(!lastRecipientp || (lastRecipientp->rank() < recipientp->rank()),
+                    AstVarScope* const vscp = acc.m_vscp;
-                        recipientp, "Merging must be on lower rank");
+                    if (!vscp->user1()) vscp->user1(++nVars);
-
+                    accesses.push_back({vscp, &mtask, acc.m_access});
-            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);
                 }
-                // 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<LogicMTask*>& 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<LogicMTask*>& 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
+        // Give the MTasks a total order consistent with their dependencies
-        // set up the OrderLogicVertex -> LogicMTask map at the same time.
+        assignSequenceNumbers();
+
+        // Gather variable accesses made by each MTask
+        std::vector<Access> 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};
+            m_scratch.clear();
-            while (LogicMTask* const mtaskp
+            for (const Access& access : accesses) {
-                   = const_cast<LogicMTask*>(static_cast<const LogicMTask*>(serialize.nextp()))) {
+                if (!access.m_access.isWriteOrRW()) continue;
-                // Compute and assign rank
+                if (!access.m_vscp->varp()->isSc()) continue;
-                uint32_t rank = 0;
+                m_scratch.push_back(access.m_mtaskp);
-                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);
             }
+            serializeMTasks(m_scratch);
         }
 
-        // Gather all variables. SystemC vars will be handled slightly specially, so keep separate.
+        // Serialize DPI calls unless user gave '--threads-dpi none'.
-        const OrderGraph& orderGraph = m_mTaskGraph.moveGraph().orderGraph();
-        std::vector<const OrderVarStdVertex*> regularVars;
-        std::vector<const OrderVarStdVertex*> 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<const OrderVarStdVertex>()) {
-                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.
         if (!v3Global.opt.threadsDpiPure() || !v3Global.opt.threadsDpiUnpure()) {
-            TasksByRank tasksByRank;
+            m_scratch.clear();
             for (V3GraphVertex& vtx : m_mTaskGraph.vertices()) {
                 LogicMTask& mtask = static_cast<LogicMTask&>(vtx);
-                if (hasDpiHazard(&mtask)) tasksByRank[mtask.rank()].insert(&mtask);
+                if (hasDpiHazard(&mtask)) m_scratch.push_back(&mtask);
             }
-            mergeSameRankTasks(tasksByRank);
+            serializeMTasks(m_scratch);
         }
     }
 

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<MTaskEdge*>(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<GraphWay::FORWARD>(edgep);
+            top->removeRelativeEdge<GraphWay::REVERSE>(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<LogicMTask*>(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<MTaskEdge&>(inEdge).fromMTaskp();
+            for (V3GraphEdge& outEdge : mtaskp->outEdges()) {
+                LogicMTask* const top = static_cast<MTaskEdge&>(outEdge).toMTaskp();
+                if (!fromp->hasEdgeTo(top)) addEdge(fromp, top);
+            }
+        }
+
+        // Remove incoming edges of 'mtaskp'
+        while (MTaskEdge* const edgep = static_cast<MTaskEdge*>(mtaskp->inEdges().frontp())) {
+            LogicMTask* const relativep = edgep->fromMTaskp();
+            relativep->removeDependent(mtaskp);
+            relativep->removeRelativeEdge<GraphWay::FORWARD>(edgep);
+            VL_DO_DANGLING(edgep->unlinkDelete(), edgep);
+        }
+        // Remove outgoing edges of 'mtaskp'
+        while (MTaskEdge* const edgep = static_cast<MTaskEdge*>(mtaskp->outEdges().frontp())) {
+            LogicMTask* const relativep = edgep->toMTaskp();
+            relativep->removeRelativeEdge<GraphWay::REVERSE>(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.

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
+    // Remove all transitive edges. (This deliberately hides V3Graph::removeTransitiveEdges,
-    // edge heaps, match those implied by the current edges of the graph, and that the graph itself
+    // which would leave the auxiliary data structures stale.)
-    // is consistent. Does nothing unless 'slowAsserts', so it is safe to call unconditionally.
+    // 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

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 <map>
 #include <memory>
 #include <unordered_map>
+#include <vector>
 
 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<LogicMTask*, VAccess, MTaskIdLessThan> m_byMTask;
+    };
+
+    // AstVarScope::user1 -> VarInfo instance for the variable (via 'varInfos')
+    const VNUser1InUse user1InUse;
+    AstUser1Allocator<AstVarScope, VarInfo> varInfos;
+
+    // The variables accessed (in enumerated order, for stability).
+    std::vector<AstVarScope*> vscps;
+
+    // Gather how each MTask accesses each variable
+    for (V3GraphVertex& vtx : mTaskGraph.vertices()) {
+        LogicMTask& mtask = static_cast<LogicMTask&>(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<OrderMTaskGraph> partition(OrderMoveGraph& moveGraph) {
     std::unique_ptr<OrderMTaskGraph> 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<OrderMTaskGraph> partition(OrderMoveGraph& moveGraph) {
         mTaskGraphp->hashGraphDebug("MTask graph after contract()");
     }
 
-    // Note the graph is only mutated by generic V3Graph algorithms from here on. These neither
+    // Remove MTasks that have no logic in them, rerouting the edges
-    // maintain the critical paths of the MTasks, nor create MTaskEdges when rerouting, so the
+    mTaskGraphp->removeEmptyMTasks();
-    // critical paths are stale below, and the graph must not be handed back to OrderMTaskGraph.
+    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
+    // Check for data hazards the partitioning left unordered
-    // mtask on every underlying OrderMoveVertex. Clear vertex lists (used later).
+    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<LogicMTask>();
         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>();
         LogicMTask* const mTaskp = const_cast<LogicMTask*>(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<const LogicMTask>();
+            // Skip the entry vertex, which has no ExecMTask
+            if (fromp == mTaskGraphp->entryp()) continue;
             new V3GraphEdge{depGraphp, logicMTaskToExecMTask.at(fromp), execMTaskp, 1};
         }
     }

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