Internals: Create if statements for triggers during scheduling (#6280) (#6581)

The AstIf nodes conditional on events being triggered used to be created
in V3Clock. Now it is in V3Sched*, in order to avoid having to pass
AstActive in CFunc or MTask bodies. No functional change intended, some
improved optimization due to simplifying timing triggers that were
previously missed, also fixes what seems like a bug in the original
timing commit code.

src/V3Clock.cpp

@@ -1,6 +1,6 @@
 // -*- mode: C++; c-file-style: "cc-mode" -*-
 //*************************************************************************
-// DESCRIPTION: Verilator: Clocking POS/NEGEDGE insertion
+// DESCRIPTION: Verilator: Post scheduling transformations
 //
 // Code available from: https://verilator.org
 //
@@ -15,15 +15,7 @@
 //*************************************************************************
 // V3Clock's Transformations:
 //
-// Top Scope:
+//  This pass is historic and does some arbitray post scheduling rewrites
-//   Check created ACTIVEs
-//      Compress adjacent ACTIVEs with same sensitivity list
-//      Form master _eval function
-//              Add around the SENTREE a (IF POSEDGE(..))
-//                      Add a __Vlast_{clock} for the comparison
-//                      Set the __Vlast_{clock} at the end of the block
-//              Replace UNTILSTABLEs with loops until specified signals become const.
-//   Create global calling function for any per-scope functions.  (For FINALs).
 //
 //*************************************************************************
 
@@ -71,32 +63,7 @@ class ClockVisitor final : public VNVisitor {
 
     // STATE
     AstCFunc* const m_evalp = nullptr;  // The '_eval' function
-    AstSenTree* m_lastSenp = nullptr;  // Last sensitivity match, so we can detect duplicates.
-    AstIf* m_lastIfp = nullptr;  // Last sensitivity if active to add more under
 
-    // METHODS
-
-    AstNodeExpr* createSenseEquation(AstSenItem* nodesp) {
-        AstNodeExpr* senEqnp = nullptr;
-        for (AstSenItem* senp = nodesp; senp; senp = VN_AS(senp->nextp(), SenItem)) {
-            UASSERT_OBJ(senp->edgeType() == VEdgeType::ET_TRUE, senp, "Should have been lowered");
-            if (senp->sensp()) {
-                AstNodeExpr* const senOnep = senp->sensp()->cloneTree(false);
-                senEqnp = senEqnp ? new AstOr{senp->fileline(), senEqnp, senOnep} : senOnep;
-            }
-        }
-        return senEqnp;
-    }
-    AstIf* makeActiveIf(AstSenTree* sentreep) {
-        AstNodeExpr* const senEqnp = createSenseEquation(sentreep->sensesp());
-        UASSERT_OBJ(senEqnp, sentreep, "No sense equation, shouldn't be in sequent activation.");
-        AstIf* const newifp = new AstIf{sentreep->fileline(), senEqnp};
-        return newifp;
-    }
-    void clearLastSen() {
-        m_lastSenp = nullptr;
-        m_lastIfp = nullptr;
-    }
     // VISITORS
     void visit(AstCoverToggle* nodep) override {
         // UINFOTREE(1, nodep, "", "ct");
@@ -125,41 +92,7 @@ class ClockVisitor final : public VNVisitor {
         VL_DO_DANGLING(nodep->deleteTree(), nodep);
     }
     void visit(AstSenTree* nodep) override {
-        nodep->unlinkFrBack();
+        pushDeletep(nodep->unlinkFrBack());  // No longer needed
-        pushDeletep(nodep);  // Delete it later, AstActives still pointing to it
-    }
-    void visit(AstActive* nodep) override {
-        UASSERT_OBJ(nodep->hasClocked(), nodep, "Should have been converted by V3Sched");
-        UASSERT_OBJ(nodep->stmtsp(), nodep, "Should not have been created if empty");
-
-        AstNode* const stmtsp = nodep->stmtsp()->unlinkFrBackWithNext();
-
-        // Create 'if' statement, if needed
-        if (!m_lastSenp || !nodep->sentreep()->sameTree(m_lastSenp)) {
-            VNRelinker relinker;
-            nodep->unlinkFrBack(&relinker);
-            clearLastSen();
-            m_lastSenp = nodep->sentreep();
-            // Make a new if statement
-            m_lastIfp = makeActiveIf(m_lastSenp);
-            relinker.relink(m_lastIfp);
-        } else {
-            nodep->unlinkFrBack();
-        }
-
-        // Move statements to if
-        m_lastIfp->addThensp(stmtsp);
-
-        // Dispose of the AstActive
-        VL_DO_DANGLING(nodep->deleteTree(), nodep);
-    }
-    void visit(AstExecGraph* nodep) override {
-        for (AstMTaskBody* mtaskBodyp = nodep->mTaskBodiesp(); mtaskBodyp;
-             mtaskBodyp = VN_AS(mtaskBodyp->nextp(), MTaskBody)) {
-            clearLastSen();
-            iterate(mtaskBodyp);
-        }
-        clearLastSen();
     }
 
     //========== Move sampled assignments
@@ -187,11 +120,6 @@ public:
     // CONSTRUCTORS
     explicit ClockVisitor(AstNetlist* netlistp)
         : m_evalp{netlistp->evalp()} {
-        // Simplify all SenTrees
-        for (AstSenTree* senTreep = netlistp->topScopep()->senTreesp(); senTreep;
-             senTreep = VN_AS(senTreep->nextp(), SenTree)) {
-            V3Const::constifyExpensiveEdit(senTreep);
-        }
         iterate(netlistp);
     }
     ~ClockVisitor() override = default;

src/V3Order.cpp

@@ -137,15 +137,13 @@ AstCFunc* V3Order::order(AstNetlist* netlistp,  //
     processDomains(netlistp, *graph, tag, externalDomains);
 
     if (parallel) {
-        // Construct the parallel ExecGraph
+        // Construct the parallel code
-        AstExecGraph* const execGraphp = createParallel(*graph, tag, trigToSen, slow);
+        AstNodeStmt* const stmtsp = createParallel(*graph, tag, trigToSen, slow);
-        // Add the ExecGraph to the result function.
+        funcp->addStmtsp(stmtsp);
-        funcp->addStmtsp(execGraphp);
     } else {
         // Construct the serial code
-        const std::vector<AstActive*> activeps = createSerial(*graph, tag, trigToSen, slow);
+        AstNodeStmt* const stmtsp = createSerial(*graph, tag, trigToSen, slow);
-        // Add the resulting Active blocks to the result function
+        funcp->addStmtsp(stmtsp);
-        for (AstNode* const nodep : activeps) funcp->addStmtsp(nodep);
     }
 
     // Dump data

src/V3OrderCFuncEmitter.h

@@ -27,6 +27,7 @@
 #include "V3Ast.h"
 #include "V3Graph.h"
 #include "V3OrderGraph.h"
+#include "V3Sched.h"
 
 #include <limits>
 #include <map>
@@ -53,8 +54,8 @@ class V3OrderCFuncEmitter final {
     AstCFunc* m_funcp = nullptr;
     // Function ordinals to ensure unique names
     std::map<std::pair<AstNodeModule*, std::string>, unsigned> m_funcNums;
-    // The result Active blocks that must be invoked to run the code in the order it was emitted
+    // The resulting ordered CFuncs with the trigger conditions needed to call them
-    std::vector<AstActive*> m_activeps;
+    std::vector<std::pair<AstCFunc*, AstSenTree*>> m_result;
 
     // Create a unique name for a new function
     std::string cfuncName(FileLine* flp, AstScope* scopep, AstNodeModule* modp,
@@ -81,10 +82,34 @@ public:
         m_funcp = nullptr;
     }
 
-    // Retrieve Active block, which when executed will call the constructed functions
+    // Retrieve list of statements which when executed will call the constructed functions
-    std::vector<AstActive*> getAndClearActiveps() {
+    AstNodeStmt* getStmts() {
+        // The resulting list of statements we are constructing here
+        AstNodeStmt* stmtsp = nullptr;
+        // Current AstIf
+        AstIf* ifp = nullptr;
+        // Trigger conditoin of 'ifp'
+        AstSenTree* pervSenTreep = nullptr;
+        // Call each function under an AstIf that checks for the trigger condition
+        for (const auto& pair : m_result) {
+            AstCFunc* const cfuncp = pair.first;
+            AstSenTree* senTreep = pair.second;
+            // Create a new AstIf if the trigger is different
+            if (senTreep != pervSenTreep) {
+                pervSenTreep = senTreep;
+                ifp = V3Sched::createIfFromSenTree(senTreep);
+                stmtsp = AstNode::addNext(stmtsp, ifp);
+            }
+            // Call function when triggered
+            AstCCall* const callp = new AstCCall{cfuncp->fileline(), cfuncp};
+            callp->dtypeSetVoid();
+            ifp->addThensp(callp->makeStmt());
+        }
+        // Result is now spent, reset the emitter state
+        m_result.clear();
         forceNewFunction();
-        return std::move(m_activeps);
+        // Return the list of statement
+        return stmtsp;
     }
 
     // Emit one logic vertex
@@ -109,7 +134,7 @@ public:
         // When profCFuncs, create a new function for each logic vertex
         if (v3Global.opt.profCFuncs()) forceNewFunction();
         // If the new domain is different, force a new function as it needs to be called separately
-        if (!m_activeps.empty() && m_activeps.back()->sentreep() != domainp) forceNewFunction();
+        if (!m_result.empty() && m_result.back().second != domainp) forceNewFunction();
 
         // Process procedures per statement, so we can split CFuncs within procedures.
         // Everything else is handled as a unit.
@@ -143,14 +168,8 @@ public:
                 m_funcp->isLoose(true);
                 m_funcp->slow(slow);
                 scopep->addBlocksp(m_funcp);
-                // Create call to the new functino
+                // Record function and sensitivity to call it with
-                AstCCall* const callp = new AstCCall{flp, m_funcp};
+                m_result.emplace_back(m_funcp, domainp);
-                callp->dtypeSetVoid();
-                // Call it under an AstActive with the same sensitivity
-                if (m_activeps.empty() || m_activeps.back()->sentreep() != domainp) {
-                    m_activeps.emplace_back(new AstActive{flp, name, domainp});
-                }
-                m_activeps.back()->addStmtsp(callp->makeStmt());
             }
             // Add the code to the current function
             m_funcp->addStmtsp(currp);

src/V3OrderInternal.h

@@ -50,15 +50,15 @@ void processDomains(AstNetlist* netlistp,  //
                     const std::string& tag,  //
                     const ExternalDomainsProvider& externalDomains);
 
-std::vector<AstActive*> createSerial(OrderGraph& orderGraph,  //
+AstNodeStmt* createSerial(OrderGraph& orderGraph,  //
-                                     const std::string& tag,  //
+                          const std::string& tag,  //
-                                     const TrigToSenMap& trigToSenMap,  //
+                          const TrigToSenMap& trigToSenMap,  //
-                                     bool slow);
+                          bool slow);
 
-AstExecGraph* createParallel(OrderGraph& orderGraph,  //
+AstNodeStmt* createParallel(OrderGraph& orderGraph,  //
-                             const std::string& tag,  //
+                            const std::string& tag,  //
-                             const TrigToSenMap& trigToSenMap,  //
+                            const TrigToSenMap& trigToSenMap,  //
-                             bool slow);
+                            bool slow);
 
 };  // namespace V3Order
 

src/V3OrderParallel.cpp

@@ -2395,8 +2395,8 @@ struct MTaskVxIdLessThan final {
     }
 };
 
-AstExecGraph* V3Order::createParallel(OrderGraph& orderGraph, const std::string& tag,
+AstNodeStmt* V3Order::createParallel(OrderGraph& orderGraph, const std::string& tag,
-                                      const TrigToSenMap& trigToSen, bool slow) {
+                                     const TrigToSenMap& trigToSen, bool slow) {
     UINFO(2, "  Constructing parallel code for '" + tag + "'");
 
     // For nondeterminism debug:
@@ -2481,7 +2481,7 @@ AstExecGraph* V3Order::createParallel(OrderGraph& orderGraph, const std::string&
         // Construct the actual MTaskBody
         AstMTaskBody* const bodyp = new AstMTaskBody{rootFlp};
         execGraphp->addMTaskBodiesp(bodyp);
-        for (AstActive* const activep : emitter.getAndClearActiveps()) bodyp->addStmtsp(activep);
+        bodyp->addStmtsp(emitter.getStmts());
         UASSERT_OBJ(bodyp->stmtsp(), bodyp, "Should not try to create empty MTask");
 
         // Create the ExecMTask

src/V3OrderSerial.cpp

@@ -31,8 +31,8 @@ VL_DEFINE_DEBUG_FUNCTIONS;
 //######################################################################
 // OrderSerial class
 
-std::vector<AstActive*> V3Order::createSerial(OrderGraph& graph, const std::string& tag,
+AstNodeStmt* V3Order::createSerial(OrderGraph& graph, const std::string& tag,
-                                              const TrigToSenMap& trigToSen, bool slow) {
+                                   const TrigToSenMap& trigToSen, bool slow) {
 
     UINFO(2, "  Constructing serial code for '" + tag + "'");
 
@@ -76,5 +76,5 @@ std::vector<AstActive*> V3Order::createSerial(OrderGraph& graph, const std::stri
     UASSERT(moveGraphp->empty(), "Waiting vertices remain, but none are ready");
     OrderMoveDomScope::clear();
 
-    return emitter.getAndClearActiveps();
+    return emitter.getStmts();
 }

src/V3Sched.cpp

@@ -39,6 +39,7 @@
 
 #include "V3Sched.h"
 
+#include "V3Const.h"
 #include "V3EmitCBase.h"
 #include "V3EmitV.h"
 #include "V3Order.h"
@@ -1050,6 +1051,10 @@ void createEval(AstNetlist* netlistp,  //
 ) {
     FileLine* const flp = netlistp->fileline();
 
+    // 'createResume' consumes the contents that 'createCommit' needs, so do the right order
+    AstCCall* const timingCommitp = timingKit.createCommit(netlistp);
+    AstCCall* const timingResumep = timingKit.createResume(netlistp);
+
     // Create the active eval loop
     const EvalLoop actLoop = createEvalLoop(  //
         netlistp, "act", "Active", /* slow: */ false, actKit.m_vscp, actKit.m_dumpp,
@@ -1060,9 +1065,7 @@ void createEval(AstNetlist* netlistp,  //
             // Compute the current 'act' triggers
             AstNodeStmt* const stmtsp = callVoidFunc(actKit.m_triggerComputep);
             // Commit trigger awaits from the previous iteration
-            if (AstCCall* const commitp = timingKit.createCommit(netlistp)) {
+            if (timingCommitp) stmtsp->addNext(timingCommitp->makeStmt());
-                stmtsp->addNext(commitp->makeStmt());
-            }
             //
             return stmtsp;
         }(),
@@ -1074,9 +1077,7 @@ void createEval(AstNetlist* netlistp,  //
             // Latch the 'act' triggers under the 'nba' triggers
             workp->addNext(createTriggerSetCall(flp, nbaKit.m_vscp, actKit.m_vscp));
             // Resume triggered timing schedulers
-            if (AstCCall* const resumep = timingKit.createResume(netlistp)) {
+            if (timingResumep) workp->addNext(timingResumep->makeStmt());
-                workp->addNext(resumep->makeStmt());
-            }
             // Invoke the 'act' function
             workp->addNext(callVoidFunc(actKit.m_funcp));
             //
@@ -1216,6 +1217,25 @@ VirtIfaceTriggers::makeMemberToSensMap(AstNetlist* const netlistp, size_t vifTri
     return memberToSensMap;
 }
 
+//============================================================================
+// Helper to build an AstIf conditional on the given SenTree being triggered
+
+AstIf* createIfFromSenTree(AstSenTree* senTreep) {
+    senTreep = VN_AS(V3Const::constifyExpensiveEdit(senTreep), SenTree);
+    UASSERT_OBJ(senTreep->sensesp(), senTreep, "No sensitivity list during scheduling");
+    // Convert the SenTree to a boolean expression that is true when triggered
+    AstNodeExpr* senEqnp = nullptr;
+    for (AstSenItem *senp = senTreep->sensesp(), *nextp; senp; senp = nextp) {
+        nextp = VN_AS(senp->nextp(), SenItem);
+        // They should all be ET_TRUE, as set up by V3Sched
+        UASSERT_OBJ(senp->edgeType() == VEdgeType::ET_TRUE, senp, "Bad scheduling trigger type");
+        AstNodeExpr* const senOnep = senp->sensp()->cloneTree(false);
+        senEqnp = senEqnp ? new AstOr{senp->fileline(), senEqnp, senOnep} : senOnep;
+    }
+    // Create the if statement conditional on the triggers
+    return new AstIf{senTreep->fileline(), senEqnp};
+}
+
 //============================================================================
 // Top level entry-point to scheduling
 

src/V3Sched.h

@@ -213,6 +213,9 @@ public:
     VirtIfaceTriggers& operator=(VirtIfaceTriggers&&) = default;
 };
 
+// Create an AstIf conditional on the given AstSenTree being triggered
+AstIf* createIfFromSenTree(AstSenTree* senTreep);
+
 // Creates trigger vars for signals driven via virtual interfaces
 VirtIfaceTriggers makeVirtIfaceTriggers(AstNetlist* nodep) VL_MT_DISABLED;
 

src/V3SchedTiming.cpp

@@ -69,20 +69,27 @@ AstCCall* TimingKit::createResume(AstNetlist* const netlistp) {
         scopeTopp->addBlocksp(m_resumeFuncp);
 
         // Put all the timing actives in the resume function
-        AstActive* dlyShedActivep = nullptr;
+        AstIf* dlyShedIfp = nullptr;
         for (auto& p : m_lbs) {
             AstActive* const activep = p.second;
             // Hack to ensure that #0 delays will be executed after any other `act` events.
             // Just handle delayed coroutines last.
             AstVarRef* const schedrefp = VN_AS(
                 VN_AS(VN_AS(activep->stmtsp(), StmtExpr)->exprp(), CMethodHard)->fromp(), VarRef);
+
+            AstIf* const ifp = V3Sched::createIfFromSenTree(activep->sentreep());
+            ifp->addThensp(activep->stmtsp()->unlinkFrBackWithNext());
+
             if (schedrefp->varScopep()->dtypep()->basicp()->isDelayScheduler()) {
-                dlyShedActivep = activep;
+                dlyShedIfp = ifp;
-                continue;
+            } else {
+                m_resumeFuncp->addStmtsp(ifp);
             }
-            m_resumeFuncp->addStmtsp(activep);
         }
-        if (dlyShedActivep) m_resumeFuncp->addStmtsp(dlyShedActivep);
+        if (dlyShedIfp) m_resumeFuncp->addStmtsp(dlyShedIfp);
+
+        // These are now spent, oispose of now empty AstActive instances
+        m_lbs.deleteActives();
     }
     AstCCall* const callp = new AstCCall{m_resumeFuncp->fileline(), m_resumeFuncp};
     callp->dtypeSetVoid();
@@ -150,15 +157,7 @@ AstCCall* TimingKit::createCommit(AstNetlist* const netlistp) {
             FileLine* const flp = senTreep->fileline();
 
             // Create an 'AstIf' sensitive to the suspending triggers
-            AstNodeExpr* senEqnp = nullptr;
+            AstIf* const ifp = V3Sched::createIfFromSenTree(senTreep);
-            for (AstSenItem* senp = senTreep->sensesp(); senp;
-                 senp = VN_AS(senp->nextp(), SenItem)) {
-                UASSERT_OBJ(senp->edgeType() == VEdgeType::ET_TRUE, senp,
-                            "Should have been lowered");
-                AstNodeExpr* const senOnep = senp->sensp()->cloneTree(false);
-                senEqnp = senEqnp ? new AstOr{senp->fileline(), senEqnp, senOnep} : senOnep;
-            }
-            AstIf* const ifp = new AstIf{flp, senEqnp};
             m_commitFuncp->addStmtsp(ifp);
 
             // Commit the processes suspended on this sensitivity expression
@@ -234,8 +233,8 @@ class AwaitVisitor final : public VNVisitor {
             postp->method(VCMethod::SCHED_DO_POST_UPDATES);
             m_postUpdatesr = AstNode::addNext(m_postUpdatesr, postp->makeStmt());
         }
-        // Put it in an active and put that in the global resume function
+        // Put it in an active
-        auto* const activep = new AstActive{flp, "_timing", sentreep};
+        AstActive* const activep = new AstActive{flp, "_timing", sentreep};
         activep->addStmtsp(resumep->makeStmt());
         m_lbs.emplace_back(m_scopeTopp, activep);
     }

src/Verilator.cpp

@@ -461,7 +461,7 @@ static void process() {
             V3Sched::schedule(v3Global.rootp());
             V3Sched::transformForks(v3Global.rootp());
 
-            // Convert sense lists into IF statements.
+            // Post scheduling transformations - TODO: this should at least be renamed
             V3Clock::clockAll(v3Global.rootp());
 
             // Cleanup any dly vars or other temps that are simple assignments