Move suspendable detection to a separate visitor (#4208)

This makes the implementation of the detection and propagation of the
suspendable property simpler and easier to read. More importantly, there are no
more jumps around the AST with the `visit` functions, which in some cases could
result in incorrect visitor context while in the `visit` function. See the added
test, which would cause Verilator to segfault before this patch.

In testing, verilation performance was not shown to be affected by this change.
Though there is a slight performance improvement from this patch, due to adding
one more check before refreshing class member cache.

Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>

docs/internals.rst

@@ -638,7 +638,22 @@ condition. See the `Timing Pass` section for more details.
 Timing Pass
 ~~~~~~~~~~~
 
-The visitor in ``V3Timing.cpp`` transforms each timing control into a ``co_await``.
+There are two visitors in ``V3Timing.cpp``.
+
+The first one, ``TimingSuspendableVisitor``, does not perform any AST
+transformations. It is responsible for marking processes and C++ functions that
+contain timing controls as suspendable. Processes that call suspendable
+functions are also marked as suspendable. Functions that call, are overridden
+by, or override suspendable functions are marked as suspendable as well.
+
+The visitor keeps a dependency graph of functions and processes to handle such
+cases. A function or process is dependent on a function if it calls it. A
+virtual class method is dependent on another class method if it calls it,
+overrides it, or is overriden by it.
+
+The second visitor in ``V3Timing.cpp``, ``TimingControlVisitor``, uses the
+information provided by ``TimingSuspendableVisitor`` and transforms each timing
+control into a ``co_await``.
 
 * event controls are turned into ``co_await`` on a trigger scheduler's
   ``trigger`` method. The awaited trigger scheduler is the one corresponding to
@@ -670,14 +685,9 @@ Each sub-statement of a ``fork`` is put in an ``AstBegin`` node for easier
 grouping. In a later step, each of these gets transformed into a new, separate
 function. See the `Forks` section for more detail.
 
-Processes that use awaits are marked as suspendable. Later, during ``V3Sched``,
+Suspendable functions get the return type of ``VlCoroutine``, which makes them
-they are transformed into coroutines. Functions that use awaits get the return
+coroutines. Later, during ``V3Sched``, suspendable processes are also
-type of ``VlCoroutine``. This immediately makes them coroutines. Note that if a
+transformed into coroutines.
-process calls a function that is a coroutine, the call gets wrapped in an
-await, which means the process itself will be marked as suspendable. A virtual
-function is a coroutine if any of its overriding or overridden functions are
-coroutines. The visitor keeps a dependency graph of functions and processes to
-handle such cases.
 
 Scheduling with timing
 ~~~~~~~~~~~~~~~~~~~~~~

src/V3Timing.cpp

@@ -12,7 +12,13 @@
 // Version 2.0.
 // SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
 //
-// TimingVisitor transformations:
+// TimingSuspendableVisitor locates all C++ functions and processes that contain timing controls,
+// and marks them as suspendable. If a process calls a suspendable function, then it is also marked
+// as suspendable. If a function calls or overrides a suspendable function, it is also marked as
+// suspendable. TimingSuspendableVisitor creates a dependency graph to propagate this property. It
+// does not perform any AST transformations.
+//
+// TimingControlVisitor is the one that actually performs transformations:
 // - for each intra-assignment timing control:
 //     - if it's a continuous assignment, transform it into an always
 //     - introduce an intermediate variable
@@ -33,9 +39,6 @@
 //     - if it's not a fork..join_none:
 //         - create a join sync variable
 //         - create statements that sync the main process with its children
-// - for each process or C++ function, if it has CAwait statements, mark it as suspendable
-//     - if we mark a virtual function as suspendable, mark all overriding and overridden functions
-//     as suspendable, as well as calling processes
 //
 // See the internals documentation docs/internals.rst for more details.
 //
@@ -57,15 +60,16 @@
 VL_DEFINE_DEBUG_FUNCTIONS;
 
 // ######################################################################
-//  Transform nodes affected by timing
+//  Detect nodes affected by timing
 
-class TimingVisitor final : public VNVisitor {
+class TimingSuspendableVisitor final : public VNVisitor {
 private:
     // TYPES
     // Vertex of a dependency graph of suspendable nodes, e.g. if a node (process or task) is
     // suspendable, all its dependents should also be suspendable
-    class DependencyVertex final : public V3GraphVertex {
+    class TimingDependencyVertex final : public V3GraphVertex {
         AstNode* const m_nodep;  // AST node represented by this graph vertex
+
         // ACCESSORS
         string name() const override VL_MT_STABLE {
             return cvtToHex(nodep()) + ' ' + nodep()->prettyTypeName();
@@ -75,29 +79,153 @@ private:
 
     public:
         // CONSTRUCTORS
-        DependencyVertex(V3Graph* graphp, AstNode* nodep)
+        TimingDependencyVertex(V3Graph* graphp, AstNode* nodep)
             : V3GraphVertex{graphp}
             , m_nodep{nodep} {}
-        ~DependencyVertex() override = default;
+        ~TimingDependencyVertex() override = default;
 
         // ACCESSORS
         virtual AstNode* nodep() const VL_MT_STABLE { return m_nodep; }
     };
 
     // NODE STATE
-    //  AstNode::user1()                         -> bool.         Set true if the node has been
+    //  AstClass::user1()                        -> bool.               Set true if the class
-    //                                                            processed.
+    //                                                                  member cache has been
-    //  AstSenTree::user1()                      -> AstVarScope*. Trigger scheduler assigned
+    //                                                                  refreshed.
-    //                                                            to this sentree
+    //  Ast{NodeProcedure,CFunc,Begin}::user2()  -> bool.               Set true if process/task is
-    //  Ast{NodeProcedure,CFunc,Begin}::user2()  -> bool.         Set true if process/task is
+    //                                                                  suspendable
-    //                                                            suspendable
-    //  AstSenTree::user2()                      -> AstCExpr*.    Debug info passed to the
-    //                                                            timing schedulers
     //  Ast{NodeProcedure,CFunc,Begin}::user3()  -> DependencyVertex*.  Vertex in m_depGraph
     const VNUser1InUse m_user1InUse;
     const VNUser2InUse m_user2InUse;
     const VNUser3InUse m_user3InUse;
 
+    // STATE
+    AstClass* m_classp = nullptr;  // Current class
+    AstNode* m_procp = nullptr;  // NodeProcedure/CFunc/Begin we're under
+    V3Graph m_depGraph;  // Dependency graph where a node is a dependency of another if it being
+                         // suspendable makes the other node suspendable
+
+    // METHODS
+    // Get or create the dependency vertex for the given node
+    TimingDependencyVertex* getDependencyVertex(AstNode* const nodep) {
+        if (!nodep->user3p()) nodep->user3p(new TimingDependencyVertex{&m_depGraph, nodep});
+        return nodep->user3u().to<TimingDependencyVertex*>();
+    }
+    // Propagate suspendable flag to all nodes that depend on the given one
+    void propagateSuspendable(TimingDependencyVertex* const vxp) {
+        for (V3GraphEdge* edgep = vxp->inBeginp(); edgep; edgep = edgep->inNextp()) {
+            auto* const depVxp = static_cast<TimingDependencyVertex*>(edgep->fromp());
+            AstNode* const depp = depVxp->nodep();
+            if (!depp->user2()) {
+                depp->user2(true);
+                propagateSuspendable(depVxp);
+            }
+        }
+    }
+
+    // VISITORS
+    void visit(AstClass* nodep) override {
+        UASSERT(!m_classp, "Class under class");
+        VL_RESTORER(m_classp);
+        m_classp = nodep;
+        iterateChildren(nodep);
+    }
+    void visit(AstNodeProcedure* nodep) override {
+        VL_RESTORER(m_procp);
+        m_procp = nodep;
+        iterateChildren(nodep);
+    }
+    void visit(AstCFunc* nodep) override {
+        VL_RESTORER(m_procp);
+        m_procp = nodep;
+        iterateChildren(nodep);
+        TimingDependencyVertex* const vxp = getDependencyVertex(nodep);
+        if (!m_classp) return;
+        // If class method (possibly overrides another method)
+        if (!m_classp->user1SetOnce()) m_classp->repairCache();
+        // Go over overridden functions
+        for (auto* cextp = m_classp->extendsp(); cextp;
+             cextp = VN_AS(cextp->nextp(), ClassExtends)) {
+            // TODO: It is possible that a methods the same name in the base class is not
+            // actually overridden by our method. If this causes a problem, traverse to the
+            // root of the inheritance hierarchy and check if the original method is virtual or
+            // not.
+            if (!cextp->classp()->user1SetOnce()) cextp->classp()->repairCache();
+            if (auto* const overriddenp
+                = VN_CAST(cextp->classp()->findMember(nodep->name()), CFunc)) {
+                if (overriddenp->user2()) {  // If it's suspendable
+                    nodep->user2(true);  // Then we are also suspendable
+                    // As both are suspendable already, there is no need to add it as our
+                    // dependency or self to its dependencies
+                } else {
+                    // Make a dependency cycle, as being suspendable should propagate both up
+                    // and down the inheritance tree
+                    TimingDependencyVertex* const overriddenVxp = getDependencyVertex(overriddenp);
+                    new V3GraphEdge{&m_depGraph, vxp, overriddenVxp, 1};
+                    new V3GraphEdge{&m_depGraph, overriddenVxp, vxp, 1};
+                }
+            }
+        }
+    }
+    void visit(AstNodeCCall* nodep) override {
+        if (nodep->funcp()->user2()) {
+            m_procp->user2(true);
+            // Both the caller and the callee are suspendable, no need to make dependency edges
+            // between them
+        } else {
+            TimingDependencyVertex* const procVxp = getDependencyVertex(m_procp);
+            TimingDependencyVertex* const funcVxp = getDependencyVertex(nodep->funcp());
+            new V3GraphEdge{&m_depGraph, procVxp, funcVxp, 1};
+            iterateChildren(nodep);
+        }
+    }
+    void visit(AstBegin* nodep) override {
+        VL_RESTORER(m_procp);
+        m_procp = nodep;
+        iterateChildren(nodep);
+    }
+    void visit(AstNode* nodep) override {
+        if (nodep->isTimingControl()) {
+            v3Global.setUsesTiming();
+            if (m_procp) m_procp->user2(true);
+        }
+        iterateChildren(nodep);
+    }
+
+    //--------------------
+    void visit(AstVar*) override {}  // Accelerate
+
+public:
+    // CONSTRUCTORS
+    explicit TimingSuspendableVisitor(AstNetlist* nodep) {
+        iterate(nodep);
+        m_depGraph.removeTransitiveEdges();
+        for (V3GraphVertex* vxp = m_depGraph.verticesBeginp(); vxp; vxp = vxp->verticesNextp()) {
+            TimingDependencyVertex* const depVxp = static_cast<TimingDependencyVertex*>(vxp);
+            if (depVxp->nodep()->user2()) propagateSuspendable(depVxp);
+        }
+        if (dumpGraphLevel() >= 6) m_depGraph.dumpDotFilePrefixed("timing_deps");
+    }
+    ~TimingSuspendableVisitor() override = default;
+};
+
+// ######################################################################
+//  Transform nodes affected by timing
+
+class TimingControlVisitor final : public VNVisitor {
+private:
+    // NODE STATE
+    //  Ast{Always,NodeCCall,Fork,NodeAssign}::user1()  -> bool.         Set true if the node has
+    //                                                                   been processed.
+    //  AstSenTree::user1()                             -> AstVarScope*. Trigger scheduler assigned
+    //                                                                   to this sentree
+    //  Ast{NodeProcedure,CFunc,Begin}::user2()         -> bool.         Set true if process/task
+    //                                                                   is suspendable
+    //  AstSenTree::user2()                             -> AstCExpr*.    Debug info passed to the
+    //                                                                   timing schedulers
+    // const VNUser1InUse m_user1InUse;      (Allocated for use in SuspendableVisitor)
+    // const VNUser2InUse m_user2InUse;      (Allocated for use in SuspendableVisitor)
+
     // STATE
     // Current context
     AstNetlist* const m_netlistp;  // Root node
@@ -105,7 +233,6 @@ private:
     AstClass* m_classp = nullptr;  // Current class
     AstScope* m_scopep = nullptr;  // Current scope
     AstActive* m_activep = nullptr;  // Current active
-    AstNode* m_procp = nullptr;  // NodeProcedure/CFunc/Fork we're under
     double m_timescaleFactor = 1.0;  // Factor to scale delays by
 
     // Unique names
@@ -128,16 +255,9 @@ private:
     AstSenTree* m_dynamicSensesp = nullptr;  // Domain to trigger if a dynamic trigger is set
 
     // Other
-    V3Graph m_depGraph;  // Dependency graph where a node is a dependency of another if it being
-                         // suspendable makes the other node suspendable
     SenTreeFinder m_finder{m_netlistp};  // Sentree finder and uniquifier
 
     // METHODS
-    // Get or create the dependency vertex for the given node
-    DependencyVertex* getDependencyVertex(AstNode* const nodep) {
-        if (!nodep->user3p()) nodep->user3p(new DependencyVertex{&m_depGraph, nodep});
-        return nodep->user3u().to<DependencyVertex*>();
-    }
     // Find net delay on the LHS of an assignment
     AstDelay* getLhsNetDelay(AstNodeAssign* nodep) const {
         bool foundWrite = false;
@@ -359,7 +479,7 @@ private:
         // Create a fork sync var
         FileLine* const flp = forkp->fileline();
         // If we're in a function, insert the sync var directly before the fork
-        AstNode* const insertBeforep = VN_IS(m_procp, CFunc) ? forkp : nullptr;
+        AstNode* const insertBeforep = m_classp ? forkp : nullptr;
         AstVarScope* forkVscp
             = createTemp(flp, forkp->name() + "__sync", getCreateForkSyncDTypep(), insertBeforep);
         unsigned joinCount = 0;  // Needed for join counter
@@ -404,105 +524,54 @@ private:
         m_activep = nullptr;
     }
     void visit(AstNodeProcedure* nodep) override {
-        VL_RESTORER(m_procp);
-        m_procp = nodep;
         iterateChildren(nodep);
         if (nodep->user2()) nodep->setSuspendable();
     }
     void visit(AstAlways* nodep) override {
-        visit(static_cast<AstNodeProcedure*>(nodep));
+        if (nodep->user1SetOnce()) return;
-        if (nodep->isSuspendable() && !nodep->user1SetOnce()) {
+        iterateChildren(nodep);
-            FileLine* const flp = nodep->fileline();
+        if (!nodep->user2()) return;
-            AstSenTree* const sensesp = m_activep->sensesp();
+        nodep->setSuspendable();
-            if (sensesp->hasClocked()) {
+        FileLine* const flp = nodep->fileline();
-                AstNode* bodysp = nodep->stmtsp()->unlinkFrBackWithNext();
+        AstSenTree* const sensesp = m_activep->sensesp();
-                auto* const controlp = new AstEventControl{flp, sensesp->cloneTree(false), bodysp};
+        if (sensesp->hasClocked()) {
-                nodep->addStmtsp(controlp);
+            AstNode* const bodysp = nodep->stmtsp()->unlinkFrBackWithNext();
-                iterate(controlp);
+            auto* const controlp = new AstEventControl{flp, sensesp->cloneTree(false), bodysp};
-            }
+            nodep->addStmtsp(controlp);
-            // Note: The 'while (true)' outer loop will be added in V3Sched
+            iterate(controlp);
-            auto* const activep = new AstActive{
-                flp, "", new AstSenTree{flp, new AstSenItem{flp, AstSenItem::Initial{}}}};
-            activep->sensesStorep(activep->sensesp());
-            activep->addStmtsp(nodep->unlinkFrBack());
-            m_activep->addNextHere(activep);
         }
+        // Note: The 'while (true)' outer loop will be added in V3Sched
+        auto* const activep = new AstActive{
+            flp, "", new AstSenTree{flp, new AstSenItem{flp, AstSenItem::Initial{}}}};
+        activep->sensesStorep(activep->sensesp());
+        activep->addStmtsp(nodep->unlinkFrBack());
+        m_activep->addNextHere(activep);
     }
     void visit(AstCFunc* nodep) override {
-        VL_RESTORER(m_procp);
-        m_procp = nodep;
         iterateChildren(nodep);
-        DependencyVertex* const vxp = getDependencyVertex(nodep);
+        if (nodep->user2()) {
-        if (m_classp && !nodep->user1SetOnce()) {  // If class method (possibly overrides another
-                                                   // method); only visit once
-            // Go over overridden functions
-            m_classp->repairCache();
-            for (auto* cextp = m_classp->extendsp(); cextp;
-                 cextp = VN_AS(cextp->nextp(), ClassExtends)) {
-                if (!cextp->classp()->user1SetOnce()) {
-                    // Repair class cache if it's not repaired already
-                    cextp->classp()->repairCache();
-                }
-                if (auto* const overriddenp
-                    = VN_CAST(cextp->classp()->findMember(nodep->name()), CFunc)) {
-                    if (overriddenp->user2()) {  // If suspendable
-                        if (!nodep->user2()) {
-                            // It should be a coroutine but it has no awaits. Add a co_return at
-                            // the end (either that or a co_await is required in a coroutine)
-                            nodep->addStmtsp(new AstCStmt{nodep->fileline(), "co_return;\n"});
-                        }
-                        nodep->user2(true);
-                        // If it's suspendable already, no need to add it as our dependency or
-                        // self to its dependencies
-                    } else {
-                        DependencyVertex* const overriddenVxp = getDependencyVertex(overriddenp);
-                        new V3GraphEdge{&m_depGraph, vxp, overriddenVxp, 1};
-                        new V3GraphEdge{&m_depGraph, overriddenVxp, vxp, 1};
-                    }
-                }
-            }
-        }
-        if (nodep->user2() && !nodep->isCoroutine()) {  // If first marked as suspendable
             nodep->rtnType("VlCoroutine");
             // If in a class, create a shared pointer to 'this'
             if (m_classp) nodep->addInitsp(new AstCStmt{nodep->fileline(), "VL_KEEP_THIS;\n"});
-            // Revisit dependent nodes if needed
+            if (!nodep->exists([](AstCAwait*) { return true; })) {
-            for (V3GraphEdge* edgep = vxp->inBeginp(); edgep; edgep = edgep->inNextp()) {
+                // It's a coroutine but has no awaits (a class method that overrides/is
-                auto* const depVxp = static_cast<DependencyVertex*>(edgep->fromp());
+                // overridden by a suspendable, but doesn't have any awaits itself). Add a
-                AstNode* const depp = depVxp->nodep();
+                // co_return at the end (either that or a co_await is required in a
-                if (!depp->user2()) {  // If dependent not suspendable
+                // coroutine)
-                    depp->user2(true);
+                nodep->addStmtsp(new AstCStmt{nodep->fileline(), "co_return;\n"});
-                    if (auto* const funcp = VN_CAST(depp, CFunc)) {
-                        // It's a coroutine but has no awaits (a class method that overrides/is
-                        // overridden by a suspendable, but doesn't have any awaits itself). Add a
-                        // co_return at the end (either that or a co_await is required in a
-                        // coroutine)
-                        funcp->addStmtsp(new AstCStmt{funcp->fileline(), "co_return;\n"});
-                    }
-                }
-                iterate(depp);
             }
         }
     }
     void visit(AstNodeCCall* nodep) override {
-        if (nodep->funcp()->user2()) {  // If suspendable
+        if (nodep->funcp()->user2() && !nodep->user1SetOnce()) {  // If suspendable
             VNRelinker relinker;
             nodep->unlinkFrBack(&relinker);
             AstCAwait* const awaitp = new AstCAwait{nodep->fileline(), nodep};
             awaitp->dtypeSetVoid();
             relinker.relink(awaitp);
-        } else {
-            // Add our process/func as the CFunc's dependency as we might have to put an await here
-            DependencyVertex* const procVxp = getDependencyVertex(m_procp);
-            DependencyVertex* const funcVxp = getDependencyVertex(nodep->funcp());
-            new V3GraphEdge{&m_depGraph, procVxp, funcVxp, 1};
         }
         iterateChildren(nodep);
     }
-    void visit(AstCAwait* nodep) override {
-        v3Global.setUsesTiming();
-        m_procp->user2(true);
-    }
     void visit(AstDelay* nodep) override {
         UASSERT_OBJ(!nodep->isCycleDelay(), nodep,
                     "Cycle delays should have been handled in V3AssertPre");
@@ -648,7 +717,7 @@ private:
         }
         // Insert new vars before the timing control if we're in a function; in a process we can't
         // do that. These intra-assignment vars will later be passed to forked processes by value.
-        AstNode* const insertBeforep = VN_IS(m_procp, CFunc) ? controlp : nullptr;
+        AstNode* const insertBeforep = m_classp ? controlp : nullptr;
         // Function for replacing values with intermediate variables
         const auto replaceWithIntermediate = [&](AstNodeExpr* const valuep,
                                                  const std::string& name) {
@@ -775,8 +844,6 @@ private:
             }
             auto* const beginp = VN_AS(stmtp, Begin);
             stmtp = beginp->nextp();
-            VL_RESTORER(m_procp);
-            m_procp = beginp;
             iterate(beginp);
             // Even if we do not find any awaits, we cannot simply inline the process here, as new
             // awaits could be added later.
@@ -787,18 +854,16 @@ private:
     }
 
     //--------------------
-    void visit(AstNodeExpr*) override {}  // Accelerate
+    void visit(AstVar*) override {}  // Accelerate
-    void visit(AstVar*) override {}
     void visit(AstNode* nodep) override { iterateChildren(nodep); }
 
 public:
     // CONSTRUCTORS
-    explicit TimingVisitor(AstNetlist* nodep)
+    explicit TimingControlVisitor(AstNetlist* nodep)
         : m_netlistp{nodep} {
         iterate(nodep);
-        if (dumpGraphLevel() >= 6) m_depGraph.dumpDotFilePrefixed("timing_deps");
     }
-    ~TimingVisitor() override = default;
+    ~TimingControlVisitor() override = default;
 };
 
 //######################################################################
@@ -806,6 +871,7 @@ public:
 
 void V3Timing::timingAll(AstNetlist* nodep) {
     UINFO(2, __FUNCTION__ << ": " << endl);
-    TimingVisitor{nodep};
+    TimingSuspendableVisitor susVisitor{nodep};
+    if (v3Global.usesTiming()) TimingControlVisitor{nodep};
     V3Global::dumpCheckGlobalTree("timing", 0, dumpTreeLevel() >= 3);
 }

test_regress/t/t_semaphore_always.pl

@@ -0,0 +1,27 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2023 by Wilson Snyder. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+scenarios(simulator => 1);
+
+if (!$Self->have_coroutines) {
+    skip("No coroutine support");
+}
+else {
+    compile(
+        verilator_flags2 => ["--timing"],
+        );
+
+    execute(
+        check_finished => 1,
+        );
+}
+
+ok(1);
+1;

test_regress/t/t_semaphore_always.v

@@ -0,0 +1,23 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Antmicro Ltd.
+// SPDX-License-Identifier: CC0-1.0
+
+module t (/*AUTOARG*/
+   // Inputs
+   clk
+   );
+   input clk;
+   semaphore s = new;
+
+   initial begin
+      s.put();
+   end
+
+   always @(posedge clk) begin
+      s.get();
+      $write("*-* All Finished *-*\n");
+      $finish;
+   end
+endmodule