From 60c532908e64a615ebe07d87e8e22f97143f8a01 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Mon, 27 Oct 2025 11:41:30 +0100 Subject: [PATCH] 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 | 78 ++------------------------------------- src/V3Order.cpp | 12 +++--- src/V3OrderCFuncEmitter.h | 47 ++++++++++++++++------- src/V3OrderInternal.h | 16 ++++---- src/V3OrderParallel.cpp | 6 +-- src/V3OrderSerial.cpp | 6 +-- src/V3Sched.cpp | 32 +++++++++++++--- src/V3Sched.h | 3 ++ src/V3SchedTiming.cpp | 31 ++++++++-------- src/Verilator.cpp | 2 +- 10 files changed, 100 insertions(+), 133 deletions(-) diff --git a/src/V3Clock.cpp b/src/V3Clock.cpp index 26fb66869..c6f68242c 100644 --- a/src/V3Clock.cpp +++ b/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: -// 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). +// This pass is historic and does some arbitray post scheduling rewrites // //************************************************************************* @@ -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); // 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(); + pushDeletep(nodep->unlinkFrBack()); // No longer needed } //========== 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; diff --git a/src/V3Order.cpp b/src/V3Order.cpp index edb42a2a7..dda6ec9ac 100644 --- a/src/V3Order.cpp +++ b/src/V3Order.cpp @@ -137,15 +137,13 @@ AstCFunc* V3Order::order(AstNetlist* netlistp, // processDomains(netlistp, *graph, tag, externalDomains); if (parallel) { - // Construct the parallel ExecGraph - AstExecGraph* const execGraphp = createParallel(*graph, tag, trigToSen, slow); - // Add the ExecGraph to the result function. - funcp->addStmtsp(execGraphp); + // Construct the parallel code + AstNodeStmt* const stmtsp = createParallel(*graph, tag, trigToSen, slow); + funcp->addStmtsp(stmtsp); } else { // Construct the serial code - const std::vector activeps = createSerial(*graph, tag, trigToSen, slow); - // Add the resulting Active blocks to the result function - for (AstNode* const nodep : activeps) funcp->addStmtsp(nodep); + AstNodeStmt* const stmtsp = createSerial(*graph, tag, trigToSen, slow); + funcp->addStmtsp(stmtsp); } // Dump data diff --git a/src/V3OrderCFuncEmitter.h b/src/V3OrderCFuncEmitter.h index c50a6fa7d..6c3d4c077 100644 --- a/src/V3OrderCFuncEmitter.h +++ b/src/V3OrderCFuncEmitter.h @@ -27,6 +27,7 @@ #include "V3Ast.h" #include "V3Graph.h" #include "V3OrderGraph.h" +#include "V3Sched.h" #include #include @@ -53,8 +54,8 @@ class V3OrderCFuncEmitter final { AstCFunc* m_funcp = nullptr; // Function ordinals to ensure unique names std::map, unsigned> m_funcNums; - // The result Active blocks that must be invoked to run the code in the order it was emitted - std::vector m_activeps; + // The resulting ordered CFuncs with the trigger conditions needed to call them + std::vector> 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 - std::vector getAndClearActiveps() { + // Retrieve list of statements which when executed will call the constructed functions + 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 - AstCCall* const callp = new AstCCall{flp, m_funcp}; - 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()); + // Record function and sensitivity to call it with + m_result.emplace_back(m_funcp, domainp); } // Add the code to the current function m_funcp->addStmtsp(currp); diff --git a/src/V3OrderInternal.h b/src/V3OrderInternal.h index 93c25c210..17d6ae177 100644 --- a/src/V3OrderInternal.h +++ b/src/V3OrderInternal.h @@ -50,15 +50,15 @@ void processDomains(AstNetlist* netlistp, // const std::string& tag, // const ExternalDomainsProvider& externalDomains); -std::vector createSerial(OrderGraph& orderGraph, // - const std::string& tag, // - const TrigToSenMap& trigToSenMap, // - bool slow); +AstNodeStmt* createSerial(OrderGraph& orderGraph, // + const std::string& tag, // + const TrigToSenMap& trigToSenMap, // + bool slow); -AstExecGraph* createParallel(OrderGraph& orderGraph, // - const std::string& tag, // - const TrigToSenMap& trigToSenMap, // - bool slow); +AstNodeStmt* createParallel(OrderGraph& orderGraph, // + const std::string& tag, // + const TrigToSenMap& trigToSenMap, // + bool slow); }; // namespace V3Order diff --git a/src/V3OrderParallel.cpp b/src/V3OrderParallel.cpp index f6ee2f506..a486b12ab 100644 --- a/src/V3OrderParallel.cpp +++ b/src/V3OrderParallel.cpp @@ -2395,8 +2395,8 @@ struct MTaskVxIdLessThan final { } }; -AstExecGraph* V3Order::createParallel(OrderGraph& orderGraph, const std::string& tag, - const TrigToSenMap& trigToSen, bool slow) { +AstNodeStmt* V3Order::createParallel(OrderGraph& orderGraph, const std::string& tag, + 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 diff --git a/src/V3OrderSerial.cpp b/src/V3OrderSerial.cpp index 23b3a7cc4..fba0295d4 100644 --- a/src/V3OrderSerial.cpp +++ b/src/V3OrderSerial.cpp @@ -31,8 +31,8 @@ VL_DEFINE_DEBUG_FUNCTIONS; //###################################################################### // OrderSerial class -std::vector V3Order::createSerial(OrderGraph& graph, const std::string& tag, - const TrigToSenMap& trigToSen, bool slow) { +AstNodeStmt* V3Order::createSerial(OrderGraph& graph, const std::string& tag, + const TrigToSenMap& trigToSen, bool slow) { UINFO(2, " Constructing serial code for '" + tag + "'"); @@ -76,5 +76,5 @@ std::vector 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(); } diff --git a/src/V3Sched.cpp b/src/V3Sched.cpp index 37ed35ece..54bccf2bf 100644 --- a/src/V3Sched.cpp +++ b/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)) { - stmtsp->addNext(commitp->makeStmt()); - } + if (timingCommitp) stmtsp->addNext(timingCommitp->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)) { - workp->addNext(resumep->makeStmt()); - } + if (timingResumep) workp->addNext(timingResumep->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 diff --git a/src/V3Sched.h b/src/V3Sched.h index d41057e57..66c960fa0 100644 --- a/src/V3Sched.h +++ b/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; diff --git a/src/V3SchedTiming.cpp b/src/V3SchedTiming.cpp index dd90633ba..cfb49881b 100644 --- a/src/V3SchedTiming.cpp +++ b/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; - continue; + dlyShedIfp = ifp; + } 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; - 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}; + AstIf* const ifp = V3Sched::createIfFromSenTree(senTreep); 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 - auto* const activep = new AstActive{flp, "_timing", sentreep}; + // Put it in an active + AstActive* const activep = new AstActive{flp, "_timing", sentreep}; activep->addStmtsp(resumep->makeStmt()); m_lbs.emplace_back(m_scopeTopp, activep); } diff --git a/src/Verilator.cpp b/src/Verilator.cpp index 3af3a56ee..892df9a1d 100644 --- a/src/Verilator.cpp +++ b/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