From 3f89bdcfaccb7bce524d4c8e82c0e166b9ba0526 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Thu, 2 May 2024 00:24:00 +0100 Subject: [PATCH] Defer conversion of set flag based AssignDlys (#5091) No functional change. Postpone the conversion of all AstAssignDlys that use the 'VdlySet' scheme for array LHSs until after the complete traversal of the netlist. The next patch takes advantage of this by using some extra information also gathered through the traversal to change the conversion. AstAssignDlys inside suspendable or fork are not deferred and are processed identical to the previous version. There are some TODOs in this patch that are fixed in the next patch. Output code perturbed due to variable ordering. MULTIDRIVEN message ordering perturbed due to processing order change. --- docs/gen/ex_MULTIDRIVEN_msg.rst | 2 +- src/V3Delayed.cpp | 210 +++++++++++++--------- test_regress/t/t_lint_multidriven_bad.out | 18 +- 3 files changed, 139 insertions(+), 91 deletions(-) diff --git a/docs/gen/ex_MULTIDRIVEN_msg.rst b/docs/gen/ex_MULTIDRIVEN_msg.rst index a2b090f34b..d8697a4a5b 100644 --- a/docs/gen/ex_MULTIDRIVEN_msg.rst +++ b/docs/gen/ex_MULTIDRIVEN_msg.rst @@ -1,6 +1,6 @@ .. comment: generated by t_lint_multidriven_bad .. code-block:: - %Warning-MULTIDRIVEN: example.v:1:22 Signal has multiple driving blocks with different clocking: 'out2' + %Warning-MULTIDRIVEN: example.v:1:22 Signal has multiple driving blocks with different clocking: 't.mem' example.v:1:7 ... Location of first driving block example.v:1:7 ... Location of other driving block diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index 25b4609939..bdbb043871 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -73,6 +73,16 @@ class DelayedVisitor final : public VNVisitor { AstConst* selWidthp = nullptr; // The width of the bit select }; + // Data required to lower AstAssignDelay later + struct Deferred final { + LhsComponents lhsComponents; // The components of the LHS + AstNodeExpr* rhsp = nullptr; // The captured value being assigned by the AstAssignDly + AstActive* activep = nullptr; // The active block the AstAssignDly is under + AstNode* insertionPointp = nullptr; // The insertion point after the AstAssignDly + std::string suffix; // Suffix to add to additional variables created + bool consecutive = false; // Record corresponds to an AstAssignDly immediately adjacent + }; + // NODE STATE // AstVarScope::user1p() -> aux // AstVar::user1() -> bool. Set true if already made warning @@ -111,13 +121,17 @@ class DelayedVisitor final : public VNVisitor { // STATE - for current visit position (use VL_RESTORER) AstActive* m_activep = nullptr; // Current activate const AstCFunc* m_cfuncp = nullptr; // Current public C Function - AstAssignDly* m_nextDlyp = nullptr; // Next delayed assignment in a list of assignments + AstAssignDly* m_nextDlyp = nullptr; // The nextp of the previous AstAssignDly AstNodeProcedure* m_procp = nullptr; // Current process bool m_inDlyLhs = false; // True in lhs of AstAssignDelay bool m_inLoop = false; // True in for loops bool m_inSuspendableOrFork = false; // True in suspendable processes and forks bool m_ignoreBlkAndNBlk = false; // Suppress delayed assignment BLKANDNBLK + // STATE - for deferred conversion + std::deque m_deferred; // Deferred AstAssignDly instances to lower at the end + AstVarScope* m_setVscp = nullptr; // The last used set flag, for reuse + // METHODS const AstNode* containingAssignment(const AstNode* nodep) { @@ -268,10 +282,12 @@ class DelayedVisitor final : public VNVisitor { return nodep; } - void createDlyOnSet(AstAssignDly* nodep) { - // Create delayed assignment - // See top of this file for transformation - // Return the new LHS for the assignment, Null = unlink + // Process the given AstAssignDly. Returns 'true' if the conversion is complete and 'nodep' can + // be deleted. Returns 'false' if the 'nodep' must be retained for later processing. + bool processAssignDly(AstAssignDly* nodep) { + // Optimize consecutiev assignments + const bool consecutive = m_nextDlyp == nodep; + m_nextDlyp = VN_CAST(nodep->nextp(), AssignDly); // Insertion point/helper for adding new statements in code order AstNode* insertionPointp = nodep; @@ -320,44 +336,13 @@ class DelayedVisitor final : public VNVisitor { arrIdxpr = capture(arrIdxpr, "VdlyDim" + std::to_string(i)); } - FileLine* const flp = nodep->fileline(); - - // Create the flag denoting an update is pending - AstVarScope* setVscp; - AstAssignPre* setInitp = nullptr; - if (nodep->user2p()) { - // Simplistic optimization. If the previous statement in same list was also an - // AstAssignDly, then we told this node (by setting m_nextDlyp->user2p below), that - // it can use the same 'VdlySet' variable rather than making a new one. This is - // good for code like: - // arrayA[0] <= something; - // arrayB[1] <= something; - setVscp = VN_AS(nodep->user2p(), VarScope); - ++m_statSharedSet; - } else { - // Create new one - const std::string name = "__VdlySet" + baseName; - setVscp = createNewVarScope(vscp, name, 1); - insert(new AstAssign{flp, new AstVarRef{flp, setVscp, VAccess::WRITE}, - new AstConst{flp, AstConst::BitTrue{}}}); - - if (!m_inSuspendableOrFork) { - // Suspendables reset __VdlySet in the AstAlwaysPost - setInitp = new AstAssignPre{flp, new AstVarRef{flp, setVscp, VAccess::WRITE}, - new AstConst{flp, 0}}; - } - } - // Tell next AstAssignDly it can share the 'VdlySet' variable - if (!m_inSuspendableOrFork && m_nextDlyp) m_nextDlyp->user2p(setVscp); - - // Create 'Post' ordered commit statements for delayed variable. We add all logic to the - // same block if it's for the same variable This ensures that multiple assignments to the - // same memory will result in correctly ordered code - the last assignment must be last. - // It also has the nice side effect of assisting cache locality. - - AstAlwaysPost* postp = nullptr; if (m_inSuspendableOrFork) { - postp = m_vscpAux(vscp).suspPostp; + // Currently we convert all NBAs in suspendable blocks immediately + // TODO: error check that deferrence was not required + FileLine* const flp = nodep->fileline(); + + // Get/Create 'Post' ordered block to commit the delayed value + AstAlwaysPost* postp = m_vscpAux(vscp).suspPostp; if (!postp) { postp = new AstAlwaysPost{flp}; if (!m_procp->user2p()) { @@ -368,52 +353,118 @@ class DelayedVisitor final : public VNVisitor { } VN_AS(m_procp->user2p(), Active)->addStmtsp(postp); } + + // Create the flag denoting an update is pending - no reuse here + AstVarScope* const setVscp = createNewVarScope(vscp, "__VdlySet" + baseName, 1); + // Set the flag here + insert(new AstAssign{flp, new AstVarRef{flp, setVscp, VAccess::WRITE}, + new AstConst{flp, AstConst::BitTrue{}}}); + // Create 'if (__VdlySet) { ... commit .. }' + AstIf* const ifp = new AstIf{flp, new AstVarRef{flp, setVscp, VAccess::READ}}; + postp->addStmtsp(ifp); + // Clear __VdlySet in the post block + ifp->addThensp(new AstAssign{flp, new AstVarRef{flp, setVscp, VAccess::WRITE}, + new AstConst{flp, 0}}); + // Finally assign the delayed value to the reconstructed LHS + AstNodeExpr* const newLhsp = reconstructLhs(lhsComponents, flp); + ifp->addThensp(new AstAssign{flp, newLhsp, rhsp}); + // The original AstAssignDly ('nodep') can be deleted + return true; } else { - postp = m_vscpAux(vscp).postp; - // Create the post block if not yet exists - if (!postp) { - // Make the new AstActive with identical sensitivity tree - AstActive* const activep - = createActiveLike(lhsComponents.refp->fileline(), m_activep); - m_vscpAux(vscp).activep = activep; - // Add the 'Post' scheduled block - postp = new AstAlwaysPost{flp}; - activep->addStmtsp(postp); - m_vscpAux(vscp).postp = postp; - } - AstActive* const activep = m_vscpAux(vscp).activep; - // Ensure the active block contains the current sensitivities - checkActiveSense(lhsComponents.refp, activep, m_activep); - // Add the initializer of the __VdlySet flag - if (setInitp) activep->addStmtsp(setInitp); + // Record this AstAssignDly for deferred processing + m_deferred.emplace_back(); + Deferred& record = m_deferred.back(); + // Populate it + record.lhsComponents = std::move(lhsComponents); + record.rhsp = rhsp; + record.activep = m_activep; + record.insertionPointp = insertionPointp; + record.suffix = std::move(baseName); + record.consecutive = consecutive; + // Note consecutive assignment for optimization + // If we inserted new statements, the original AstAssignDly ('nodep') + // can be deleted. Otherwise, it must be kept as a placeholder. + return insertionPointp != nodep; } + } - // Build/Get 'if (__VdlySet) { ... commit .. }' + // Convert the given deferred assignment for the given AstVarScope + void convertDeferred(Deferred& record) { + // Unpack all the parts + LhsComponents lhsComponents = std::move(record.lhsComponents); + AstActive*& oActivep = record.activep; // Original AstActive the AstAssignDly was under + AstNode* insertionPointp = record.insertionPointp; + const auto insert = [&insertionPointp](AstNode* stmtp) { + insertionPointp->addNextHere(stmtp); + insertionPointp = stmtp; + }; + // If the original AstAssignDly was kept as a placeholder, we will need to delete it + AstAssignDly* const dlyp = VN_CAST(insertionPointp, AssignDly); + + FileLine* const flp = insertionPointp->fileline(); + AstVarScope* const vscp = lhsComponents.refp->varScopep(); + + // Get/Create 'Post' ordered block to commit the delayed value + AstAlwaysPost* const postp = [&]() { + if (AstAlwaysPost* const p = m_vscpAux(vscp).postp) return p; + // Make the new AstActive with identical sensitivity tree + AstActive* const activep = createActiveLike(lhsComponents.refp->fileline(), oActivep); + m_vscpAux(vscp).activep = activep; + // Add the 'Post' scheduled block + AstAlwaysPost* const p = new AstAlwaysPost{flp}; + activep->addStmtsp(p); + m_vscpAux(vscp).postp = p; + return p; + }(); + // Pick up the active block containing 'postp' + AstActive* const activep = m_vscpAux(vscp).activep; + // Ensure it contains the current sensitivities + checkActiveSense(lhsComponents.refp, activep, oActivep); + + // Create the flag denoting an update is pending + if (record.consecutive) { + // Simplistic optimization. If the previous assignment was immediately before this + // assignment, we can reuse the existing flag. This is good for code like: + // arrayA[0] <= something; + // arrayB[1] <= something; + ++m_statSharedSet; + } else { + // Create new flag + m_setVscp = createNewVarScope(vscp, "__VdlySet" + record.suffix, 1); + // Set it here + insert(new AstAssign{flp, new AstVarRef{flp, m_setVscp, VAccess::WRITE}, + new AstConst{flp, AstConst::BitTrue{}}}); + // Add the 'Pre' ordered reset for the flag + activep->addStmtsp(new AstAssignPre{flp, new AstVarRef{flp, m_setVscp, VAccess::WRITE}, + new AstConst{flp, 0}}); + }; + + // Create/Get 'if (__VdlySet) { ... commit .. }' AstIf* ifp = nullptr; - if (postp->user2p() == setVscp) { - // Optimize as above. If sharing VdlySet *ON SAME VARIABLE*, we can share the AstIf + if (postp->user2p() == m_setVscp) { + // Optimize as above. If sharing VdlySet *ON SAME VARIABLE*, we can share the 'if' ifp = VN_AS(postp->user1p(), If); } else { - ifp = new AstIf{flp, new AstVarRef{flp, setVscp, VAccess::READ}}; + ifp = new AstIf{flp, new AstVarRef{flp, m_setVscp, VAccess::READ}}; postp->addStmtsp(ifp); postp->user1p(ifp); // Remember the associated 'AstIf' - postp->user2p(setVscp); // Remember the VdlySet variable used as the condition. - } - - // Suspendables clear __VdlySet in the post block - if (m_inSuspendableOrFork) { - ifp->addThensp(new AstAssign{flp, new AstVarRef{flp, setVscp, VAccess::WRITE}, - new AstConst{flp, 0}}); + postp->user2p(m_setVscp); // Remember the VdlySet variable used as the condition. } - // Reconstruct the delayed LHS expression + // Finally assign the delayed value to the reconstructed LHS AstNodeExpr* const newLhsp = reconstructLhs(lhsComponents, flp); - // Finally assign the delayed value - ifp->addThensp(new AstAssign{flp, newLhsp, rhsp}); + ifp->addThensp(new AstAssign{flp, newLhsp, record.rhsp}); + + // If the original AstAssignDelay was kept as a placeholder, we can nuke it now. + if (dlyp) pushDeletep(dlyp->unlinkFrBack()); } // VISITORS - void visit(AstNetlist* nodep) override { iterateChildren(nodep); } + void visit(AstNetlist* nodep) override { + iterateChildren(nodep); + // Convert all AstAssignDelay instances that were not converted during the traversal + for (Deferred& record : m_deferred) convertDeferred(record); + } void visit(AstScope* nodep) override { AstNode::user2ClearTree(); iterateChildren(nodep); @@ -518,9 +569,6 @@ class DelayedVisitor final : public VNVisitor { void visit(AstAlwaysPost*) override {} void visit(AstAssignDly* nodep) override { - VL_RESTORER(m_nextDlyp); - // Next assignment in same block, maybe nullptr. - m_nextDlyp = VN_CAST(nodep->nextp(), AssignDly); if (m_cfuncp) { if (!v3Global.rootp()->nbaEventp()) { nodep->v3warn( @@ -542,11 +590,11 @@ class DelayedVisitor final : public VNVisitor { // TODO: this message is not covered by tests if (basicp->isEvent()) nodep->v3warn(E_UNSUPPORTED, "Unsupported: event arrays"); } - - createDlyOnSet(nodep); - VL_DO_DANGLING(pushDeletep(nodep->unlinkFrBack()), nodep); + if (processAssignDly(nodep)) VL_DO_DANGLING(pushDeletep(nodep->unlinkFrBack()), nodep); } else if (m_inSuspendableOrFork) { - createDlyOnSet(nodep); + const bool converted = processAssignDly(nodep); + // Current implementation always converts these + UASSERT_OBJ(converted, nodep, "NBA in suspendable processes should have be converted"); VL_DO_DANGLING(pushDeletep(nodep->unlinkFrBack()), nodep); } else { { diff --git a/test_regress/t/t_lint_multidriven_bad.out b/test_regress/t/t_lint_multidriven_bad.out index 231a2368c0..788f630328 100644 --- a/test_regress/t/t_lint_multidriven_bad.out +++ b/test_regress/t/t_lint_multidriven_bad.out @@ -1,12 +1,3 @@ -%Warning-MULTIDRIVEN: t/t_lint_multidriven_bad.v:21:22: Signal has multiple driving blocks with different clocking: 't.mem' - t/t_lint_multidriven_bad.v:27:7: ... Location of first driving block - 27 | mem[a0] <= d1; - | ^~~ - t/t_lint_multidriven_bad.v:24:7: ... Location of other driving block - 24 | mem[a0] <= d0; - | ^~~ - ... For warning description see https://verilator.org/warn/MULTIDRIVEN?v=latest - ... Use "/* verilator lint_off MULTIDRIVEN */" and lint_on around source to disable this message. %Warning-MULTIDRIVEN: t/t_lint_multidriven_bad.v:19:22: Signal has multiple driving blocks with different clocking: 'out2' t/t_lint_multidriven_bad.v:35:7: ... Location of first driving block 35 | out2[15:8] <= d0; @@ -14,4 +5,13 @@ t/t_lint_multidriven_bad.v:32:7: ... Location of other driving block 32 | out2[7:0] <= d0; | ^~~~ + ... For warning description see https://verilator.org/warn/MULTIDRIVEN?v=latest + ... Use "/* verilator lint_off MULTIDRIVEN */" and lint_on around source to disable this message. +%Warning-MULTIDRIVEN: t/t_lint_multidriven_bad.v:21:22: Signal has multiple driving blocks with different clocking: 't.mem' + t/t_lint_multidriven_bad.v:27:7: ... Location of first driving block + 27 | mem[a0] <= d1; + | ^~~ + t/t_lint_multidriven_bad.v:24:7: ... Location of other driving block + 24 | mem[a0] <= d0; + | ^~~ %Error: Exiting due to