diff --git a/.github/workflows/rtlmeter.yml b/.github/workflows/rtlmeter.yml index a2b7df5a6..e4d7a9eb3 100644 --- a/.github/workflows/rtlmeter.yml +++ b/.github/workflows/rtlmeter.yml @@ -112,7 +112,7 @@ jobs: - "VeeR-EL2:default*" - "VeeR-EL2:hiperf*" - "Vortex:mini:*" - - "Vortex:sane:* !Vortex:sane:sgemm" + - "Vortex:sane:*" - "XiangShan:default-chisel3:* !*:linux" - "XiangShan:default-chisel6:* !*:linux" - "XiangShan:mini-chisel3:* !*:linux" diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index fe32bf4ca..11257859d 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -211,23 +211,32 @@ List Of Warnings public task, or when the blocking and non-blocking assignments have non-overlapping bits and structure members. + From Verilator 5.038, this warning is only issued when Verilator can't prove that + the assignments are to non-overlapping sub-parts, and the blocking + assignment is in combinational logic (which is the case where simulation + results might differ from other simulators). Review any BLKANDNBLK + cases carefully after this version, and sign them off as + described above, only if know for sure the updates are not to overlapping + parts of the signal. + Generally, this is caused by a register driven by both combo logic and a flop: .. code-block:: sv - logic [1:0] foo; - always @(posedge clk) foo[0] <= ... - always_comb foo[1] = ... + logic [3:0] foo; + always @(posedge clk) foo[index] <= ... // With index != 0 + always_comb foo[0] = ... Instead, use a different register for the flop: .. code-block:: sv - logic [1:0] foo; - always @(posedge clk) foo_flopped[0] <= ... - always_comb foo[0] = foo_flopped[0]; - always_comb foo[1] = ... + logic [3:0] foo; + logic [3:1] foo_flopped; + always @(posedge clk) foo_flopped[index] <= ... // With index != 0 + always_comb foo[0] = ... + always_comb foo[3:1] = foo_flopped; Or, this may also avoid the error: diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index 6dfe7d32e..d72ebcd9b 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -1915,6 +1915,7 @@ class AstVar final : public AstNode { bool m_isForcedByCode : 1; // May be forced/released from AstAssignForce/AstRelease bool m_isWrittenByDpi : 1; // This variable can be written by a DPI Export bool m_isWrittenBySuspendable : 1; // This variable can be written by a suspendable process + bool m_ignorePostRead : 1; // Ignore reads in 'Post' blocks during ordering bool m_ignorePostWrite : 1; // Ignore writes in 'Post' blocks during ordering bool m_ignoreSchedWrite : 1; // Ignore writes in scheduling (for special optimizations) @@ -1961,6 +1962,7 @@ class AstVar final : public AstNode { m_isForcedByCode = false; m_isWrittenByDpi = false; m_isWrittenBySuspendable = false; + m_ignorePostRead = false; m_ignorePostWrite = false; m_ignoreSchedWrite = false; m_attrClocker = VVarAttrClocker::CLOCKER_UNKNOWN; @@ -2118,6 +2120,8 @@ public: void setWrittenByDpi() { m_isWrittenByDpi = true; } bool isWrittenBySuspendable() const { return m_isWrittenBySuspendable; } void setWrittenBySuspendable() { m_isWrittenBySuspendable = true; } + bool ignorePostRead() const { return m_ignorePostRead; } + void setIgnorePostRead() { m_ignorePostRead = true; } bool ignorePostWrite() const { return m_ignorePostWrite; } void setIgnorePostWrite() { m_ignorePostWrite = true; } bool ignoreSchedWrite() const { return m_ignoreSchedWrite; } diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index e8c8e5203..0ba1758d9 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -71,6 +71,18 @@ // if (__VdlySet__LHS) a[__VdlyDim0__LHS][__VdlyDim1__LHS] = __VdlyVal__LHS; // Multiple consecutive NBAs of compatible form can share the same __VdlySet* flag // +// "Shadow variable masked" scheme. Used for packed target variables that +// have both blocking and non-blocking updates. E.g.: +// LHS[Index] <= RHS; +// When there is also LHS[SomeNonOverlappingIndex] = RHS2; +// is converted to: +// - In the original logic, replace the AstAssignDelay with: +// __Vdly__LHS[Index] = RHS; +// __VdlyMask__LHS[Index] = '1; +// - Add new "Post-scheduled" logic: +// LHS = (__Vdly__LHS & __VdlyMask__LHS) | (LHS & ~__VdlyMask__LHS); +// __VdlyMask__LHS = '0; +// // "Unique flag" scheme. Used for all variables updated by NBAs // in suspendable processees or forks. E.g.: // #1 LHS <= RHS; @@ -124,6 +136,7 @@ class DelayedVisitor final : public VNVisitor { Undecided = 0, UnsupportedCompoundArrayInLoop, ShadowVar, + ShadowVarMasked, FlagShared, FlagUnique, ValueQueueWhole, @@ -152,6 +165,10 @@ class DelayedVisitor final : public VNVisitor { struct { // Stuff needed for Scheme::ShadowVar AstVarScope* vscp; // The shadow variable } m_shadowVariableKit; + struct { // Stuff needed for Scheme::ShadowVarMasked + AstVarScope* vscp; // The shadow variable + AstVarScope* maskp; // The mask variable + } m_shadowVarMaskedKit; struct { // Stuff needed for Scheme::FlagShared AstActive* activep; // The active block for the Pre/Post logic AstAlwaysPost* postp; // The post block for commiting results @@ -174,6 +191,10 @@ class DelayedVisitor final : public VNVisitor { UASSERT(m_scheme == Scheme::ShadowVar, "Inconsistent Scheme"); return m_kitUnion.m_shadowVariableKit; } + auto& shadowVarMaskedKit() { + UASSERT(m_scheme == Scheme::ShadowVarMasked, "Inconsistent Scheme"); + return m_kitUnion.m_shadowVarMaskedKit; + } auto& flagSharedKit() { UASSERT(m_scheme == Scheme::FlagShared, "Inconsistent Scheme"); return m_kitUnion.m_flagSharedKit; @@ -202,6 +223,18 @@ class DelayedVisitor final : public VNVisitor { void addSensitivity(AstSenTree* nodep) { addSensitivity(nodep->sensesp()); } }; + // Data structure to keep track of all writes to + struct WriteReference final { + AstVarRef* m_refp = nullptr; // The reference + bool m_isNBA = false; // True if an NBA write + bool m_inNonComb = false; // True if reference is known to be in non-combinational logic + WriteReference() = default; + WriteReference(AstVarRef* refp, bool isNBA, bool inNonComb) + : m_refp{refp} + , m_isNBA{isNBA} + , m_inNonComb{inNonComb} {} + }; + // Data required to lower AstAssignDelay later struct NBA final { AstAssignDly* nodep = nullptr; // The NBA this record refers to @@ -215,10 +248,13 @@ class DelayedVisitor final : public VNVisitor { // AstNodeModule::user1p() -> std::unorded_map temp map via m_varMap // AstVarScope::user1p() -> VarScopeInfo via m_vscpInfo // AstVarScope::user2p() -> AstVarRef*: First write reference to the Variable + // AstVarScope::user3p() -> std::vector via m_writeRefs; const VNUser1InUse m_user1InUse{}; const VNUser2InUse m_user2InUse{}; + const VNUser3InUse m_user3InUse{}; AstUser1Allocator> m_varMap; AstUser1Allocator m_vscpInfo; + AstUser3Allocator> m_writeRefs; // STATE - across all visitors std::set m_timingDomains; // Timing resume domains @@ -230,6 +266,7 @@ class DelayedVisitor final : public VNVisitor { 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 + bool m_inNonCombLogic = false; // We are in non-combinational logic AstVarRef* m_currNbaLhsRefp = nullptr; // Current NBA LHS variable reference // STATE - during NBA conversion (after visit) @@ -240,6 +277,7 @@ class DelayedVisitor final : public VNVisitor { // STATE - Statistic tracking VDouble0 m_nSchemeShadowVar; // Number of variables using Scheme::ShadowVar + VDouble0 m_nSchemeShadowVarMasked; // Number of variabels using Scheme::ShadowVarMasked VDouble0 m_nSchemeFlagShared; // Number of variables using Scheme::FlagShared VDouble0 m_nSchemeFlagUnique; // Number of variables using Scheme::FlagUnique VDouble0 m_nSchemeValueQueuesWhole; // Number of variables using Scheme::ValueQueueWhole @@ -248,15 +286,129 @@ class DelayedVisitor final : public VNVisitor { // METHODS + // Return true iff a variable is assigned by both blocking and nonblocking + // assignments. Issue BLKANDNBLK error if we can't prove the mixed + // assignments are to independent bits and the blocking assignment can be + // in combinational logic, which is something we can't safely implement + // still. + bool checkMixedUsage(const AstVarScope* vscp, bool isIntegralOrPacked) { + + struct Ref final { + AstVarRef* m_refp; // The reference + bool m_inNonComb; // True if known to be in non-combinational logic + int m_lsb; // LSB of accessed range + int m_msb; // MSB of accessed range + Ref(AstVarRef* refp, bool inNonComb, int lsb, int msb) + : m_refp{refp} + , m_inNonComb{inNonComb} + , m_lsb{lsb} + , m_msb{msb} {} + }; + + std::vector blkRefs; // Blocking writes + std::vector nbaRefs; // Non-blockign writes + + const int width = isIntegralOrPacked ? vscp->width() : 1; + + for (const auto& writeRef : m_writeRefs(vscp)) { + int lsb = 0; + int msb = width - 1; + if (const AstSel* const selp = VN_CAST(writeRef.m_refp->backp(), Sel)) { + if (VN_IS(selp->lsbp(), Const)) { + lsb = selp->lsbConst(); + msb = selp->msbConst(); + } + } + if (writeRef.m_isNBA) { + nbaRefs.emplace_back(writeRef.m_refp, writeRef.m_inNonComb, lsb, msb); + } else { + blkRefs.emplace_back(writeRef.m_refp, writeRef.m_inNonComb, lsb, msb); + } + } + // We only run this function on targets of NBAs, so there should be at least one... + UASSERT_OBJ(!nbaRefs.empty(), vscp, "Did not record NBA write"); + // If no blocking upadte, then we are good + if (blkRefs.empty()) return false; + + // If the blocking assignment is in non-combinational logic (i.e.: + // in logic that has an explicit trigger), then we can safely + // implement it (there is no race between clocked logic and post + // scheduled logic), so need not error + blkRefs.erase(std::remove_if(blkRefs.begin(), blkRefs.end(), + [](const Ref& ref) { return ref.m_inNonComb; }), + blkRefs.end()); + + // If nothing left, then we need not error + if (blkRefs.empty()) return true; + + // If not a packed variable, warn here as we can't prove independence + if (!isIntegralOrPacked) { + const Ref& blkRef = blkRefs.front(); + const Ref& nbaRef = nbaRefs.front(); + vscp->v3warn( + BLKANDNBLK, + "Unsupported: Blocking and non-blocking assignments to same non-packed variable: " + << vscp->varp()->prettyNameQ() << '\n' + << vscp->warnContextPrimary() << '\n' + << blkRef.m_refp->warnOther() << "... Location of blocking assignment\n" + << blkRef.m_refp->warnContextSecondary() << '\n' + << nbaRef.m_refp->warnOther() << "... Location of nonblocking assignment\n" + << nbaRef.m_refp->warnContextSecondary()); + return true; + } + + // We need to error if we can't prove the written bits are independent + + // Sort refs by interval + const auto lessThanRef = [](const Ref& a, const Ref& b) { + if (a.m_lsb != b.m_lsb) return a.m_lsb < b.m_lsb; + return a.m_msb < b.m_msb; + }; + std::stable_sort(blkRefs.begin(), blkRefs.end(), lessThanRef); + std::stable_sort(nbaRefs.begin(), nbaRefs.end(), lessThanRef); + // Iterate both vectors, checking for overlap + auto bIt = blkRefs.begin(); + auto nIt = nbaRefs.begin(); + while (bIt != blkRefs.end() && nIt != nbaRefs.end()) { + if (lessThanRef(*bIt, *nIt)) { + if (nIt->m_lsb <= bIt->m_msb) break; // Stop on Overlap + ++bIt; + } else { + if (bIt->m_lsb <= nIt->m_msb) break; // Stop on Overlap + ++nIt; + } + } + + // If we found an overlapping range that cannot be safely implemented, then wran... + if (bIt != blkRefs.end() && nIt != nbaRefs.end()) { + const Ref& blkRef = *bIt; + const Ref& nbaRef = *nIt; + vscp->v3warn(BLKANDNBLK, + "Unsupported: Blocking and non-blocking assignments to " + "potentially overlapping bits of same packed variable: " + << vscp->varp()->prettyNameQ() << '\n' + << vscp->warnContextPrimary() << '\n' + << blkRef.m_refp->warnOther() << "... Location of blocking assignment" + << " (bits [" << blkRef.m_msb << ":" << blkRef.m_lsb << "])\n" + << blkRef.m_refp->warnContextSecondary() << '\n' + << nbaRef.m_refp->warnOther() + << "... Location of nonblocking assignment" + << " (bits [" << nbaRef.m_msb << ":" << nbaRef.m_lsb << "])\n" + << nbaRef.m_refp->warnContextSecondary()); + } + + return true; + } + // Choose the NBA scheme used for the given variable. - static Scheme chooseScheme(const AstVarScope* vscp, const VarScopeInfo& vscpInfo) { + Scheme chooseScheme(const AstVarScope* vscp, const VarScopeInfo& vscpInfo) { UASSERT_OBJ(vscpInfo.m_scheme == Scheme::Undecided, vscp, "NBA scheme already decided"); const AstNodeDType* const dtypep = vscp->dtypep()->skipRefp(); // Unpacked arrays if (const AstUnpackArrayDType* const uaDTypep = VN_CAST(dtypep, UnpackArrayDType)) { // Basic underlying type of elements, if any. - AstBasicDType* const basicp = uaDTypep->basicp(); + const AstBasicDType* const basicp = uaDTypep->basicp(); // If used in a loop, we must have a dynamic commit queue. (Also works in suspendables) if (vscpInfo.m_inLoop) { // Arrays with compound element types are currently not supported in loops @@ -281,6 +433,25 @@ class DelayedVisitor final : public VNVisitor { // In a suspendable of fork, we must use the unique flag scheme, TODO: why? if (vscpInfo.m_inSuspOrFork) return Scheme::FlagUnique; + + const bool isIntegralOrPacked = dtypep->isIntegralOrPacked(); + // Check for mixed usage (this also warns if not OK) + if (checkMixedUsage(vscp, isIntegralOrPacked)) { + // If it's a variable updated by both blocking and non-blocking + // asignments, use the ShadowVarMasked schem if masked update is + // possible. This can handle blocking and non-blocking updates to + // inpdendent parts correctly at run-time, and always works, even + // in loops or other dynamic context. + if (isIntegralOrPacked) return Scheme::ShadowVarMasked; + // If it's inside a loop, use Scheme::ShadowVar, which is safe, + // but will generate incorrect code if a partial update is used + if (vscpInfo.m_inLoop) return Scheme::ShadowVar; + // Otherwise (for not packed variables), use the FlagUnique scheme, + // which at least handles partial updates correctly, but might break + // in loops or other dynamic context + return Scheme::FlagUnique; + } + // Otherwise use the simple shadow variable scheme return Scheme::ShadowVar; } @@ -350,7 +521,12 @@ class DelayedVisitor final : public VNVisitor { nodep = arrSelp->fromp(); } // What remains must be an AstVarRef, or some sort of select, we assume can reuse it. - UASSERT_OBJ(nodep->isPure(), lhsp, "Malformed LHS in NBA"); + if (const AstAssocSel* const aselp = VN_CAST(nodep, AssocSel)) { + UASSERT_OBJ(aselp->fromp()->isPure() && aselp->bitp()->isPure(), lhsp, + "Malformed LHS in NBA"); + } else { + UASSERT_OBJ(nodep->isPure(), lhsp, "Malformed LHS in NBA"); + } // Now have been converted to use the captured values return lhsp; } @@ -410,6 +586,70 @@ class DelayedVisitor final : public VNVisitor { }); } + // Scheme::ShadowVarMasked + void prepareSchemeShadowVarMasked(AstVarScope* vscp, VarScopeInfo& vscpInfo) { + UASSERT_OBJ(vscpInfo.m_scheme == Scheme::ShadowVarMasked, vscp, "Inconsistent NBA scheme"); + FileLine* const flp = vscp->fileline(); + AstScope* const scopep = vscp->scopep(); + // Create the shadow variable + const std::string shadowName = "__Vdly__" + vscp->varp()->shortName(); + AstVarScope* const shadowVscp = createTemp(flp, scopep, shadowName, vscp->dtypep()); + vscpInfo.shadowVarMaskedKit().vscp = shadowVscp; + // Create the makk variable + const std::string maskName = "__VdlyMask__" + vscp->varp()->shortName(); + AstVarScope* const maskVscp = createTemp(flp, scopep, maskName, vscp->dtypep()); + maskVscp->varp()->setIgnorePostWrite(); + vscpInfo.shadowVarMaskedKit().maskp = maskVscp; + // Create the AstActive for the Post logic + AstActive* const activep + = new AstActive{flp, "nba-shadow-var-masked", vscpInfo.senTreep()}; + activep->sensesStorep(vscpInfo.senTreep()); + scopep->addBlocksp(activep); + // Add 'Post' scheduled process for the commit and mask clear + AstAlwaysPost* const postp = new AstAlwaysPost{flp}; + activep->addStmtsp(postp); + // Add the commit - vscp = (shadowVscp & maskVscp) | (vscp & ~maskVscp); + postp->addStmtsp(new AstAssign{ + flp, new AstVarRef{flp, vscp, VAccess::WRITE}, + new AstOr{flp, + new AstAnd{flp, new AstVarRef{flp, shadowVscp, VAccess::READ}, + new AstVarRef{flp, maskVscp, VAccess::READ}}, + new AstAnd{flp, new AstVarRef{flp, vscp, VAccess::READ}, + new AstNot{flp, new AstVarRef{flp, maskVscp, VAccess::READ}}}}}); + vscp->varp()->setIgnorePostRead(); + // Clar the mask - maskVscp = '0; + postp->addStmtsp( + new AstAssign{flp, new AstVarRef{flp, maskVscp, VAccess::WRITE}, + new AstConst{flp, AstConst::WidthedValue{}, maskVscp->width(), 0}}); + } + void convertSchemeShadowVarMasked(AstAssignDly* nodep, AstVarScope* vscp, + VarScopeInfo& vscpInfo) { + UASSERT_OBJ(vscpInfo.m_scheme == Scheme::ShadowVarMasked, vscp, "Inconsistent NBA scheme"); + AstVarScope* const shadowVscp = vscpInfo.shadowVarMaskedKit().vscp; + AstVarScope* const maskVscp = vscpInfo.shadowVarMaskedKit().maskp; + + AstNodeExpr* lhsClonep = nodep->lhsp()->cloneTree(false); + + // Replace the write ref on the LHS with the shadow variable + nodep->lhsp()->foreach([&](AstVarRef* const refp) { + if (!refp->access().isWriteOnly()) return; + UASSERT_OBJ(refp->varScopep() == vscp, nodep, "NBA not setting expected variable"); + refp->varScopep(shadowVscp); + refp->varp(shadowVscp->varp()); + }); + // Set the same bits in the mask to 1 + lhsClonep->foreach([&](AstVarRef* const refp) { + if (!refp->access().isWriteOnly()) return; + UASSERT_OBJ(refp->varScopep() == vscp, nodep, "NBA not setting expected variable"); + refp->varScopep(maskVscp); + refp->varp(maskVscp->varp()); + }); + FileLine* const flp = nodep->fileline(); + AstConst* const onesp = new AstConst{flp, AstConst::DTyped{}, lhsClonep->dtypep()}; + onesp->num().setAllBits1(); + nodep->addNextHere(new AstAssign{flp, lhsClonep, onesp}); + } + // Scheme::FlagShared void prepareSchemeFlagShared(AstVarScope* vscp, VarScopeInfo& vscpInfo) { UASSERT_OBJ(vscpInfo.m_scheme == Scheme::FlagShared, vscp, "Inconsistent NBA scheme"); @@ -705,51 +945,19 @@ class DelayedVisitor final : public VNVisitor { pushDeletep(nodep->unlinkFrBack()); } - // Record and warn if a variable is assigned by both blocking and nonblocking assignments - void checkVarUsage(AstVarRef* nodep, bool nonBlocking) { + // Record where a variable is assigned + void recordWriteRef(AstVarRef* nodep, bool nonBlocking) { // Ignore references in certain contexts if (m_ignoreBlkAndNBlk) return; - // Ignore if warning is disabled on this reference (used by V3Force). - if (nodep->fileline()->warnIsOff(V3ErrorCode::BLKANDNBLK)) return; // Ignore if it's an array // TODO: we do this because it used to be the previous behaviour. // Is it still required, or should we warn for arrays as well? // Scheduling is no different for them... + // Clarification: This is OK for arrays of primitive types, but + // arrays that use the ShadowVar scheme don't work... if (VN_IS(nodep->varScopep()->dtypep()->skipRefp(), UnpackArrayDType)) return; - // Mark ref as blocking/non-blocking - nodep->user1(nonBlocking); - - AstVarScope* const vscp = nodep->varScopep(); - - // Pick up/set the first reference to this variable - const AstVarRef* const firstRefp = VN_AS(vscp->user2p(), VarRef); - if (!firstRefp) { - vscp->user2p(nodep); - return; - } - - // If both blocking/non-blocking, it's OK - if (firstRefp->user1() == static_cast(nonBlocking)) return; - - // Otherwise warn that both blocking and non-blocking assignments are used - const auto containingAssignment = [](const AstNode* nodep) -> const AstNode* { - while (!VN_IS(nodep, NodeAssign)) nodep = nodep->backp(); - return nodep; - }; - - const AstNode* nonblockingp = nonBlocking ? nodep : firstRefp; - if (const AstNode* np = containingAssignment(nonblockingp)) nonblockingp = np; - const AstNode* blockingp = nonBlocking ? firstRefp : nodep; - if (const AstNode* np = containingAssignment(blockingp)) blockingp = np; - vscp->v3warn(BLKANDNBLK, - "Unsupported: Blocked and non-blocking assignments to same variable: " - << vscp->varp()->prettyNameQ() << '\n' - << vscp->warnContextPrimary() << '\n' - << blockingp->warnOther() << "... Location of blocking assignment\n" - << blockingp->warnContextSecondary() << '\n' - << nonblockingp->warnOther() << "... Location of nonblocking assignment\n" - << nonblockingp->warnContextSecondary()); + m_writeRefs(nodep->varScopep()).emplace_back(nodep, nonBlocking, m_inNonCombLogic); } // VISITORS @@ -773,6 +981,11 @@ class DelayedVisitor final : public VNVisitor { prepareSchemeShadowVar(vscp, vscpInfo); break; } + case Scheme::ShadowVarMasked: { + ++m_nSchemeShadowVarMasked; + prepareSchemeShadowVarMasked(vscp, vscpInfo); + break; + } case Scheme::FlagShared: { ++m_nSchemeFlagShared; prepareSchemeFlagShared(vscp, vscpInfo); @@ -816,6 +1029,10 @@ class DelayedVisitor final : public VNVisitor { convertSchemeShadowVar(nbap, vscp, vscpInfo); break; } + case Scheme::ShadowVarMasked: { + convertSchemeShadowVarMasked(nbap, vscp, vscpInfo); + break; + } case Scheme::FlagShared: { convertSchemeFlagShared(nbap, vscp, vscpInfo); break; @@ -844,9 +1061,11 @@ class DelayedVisitor final : public VNVisitor { UASSERT_OBJ(!m_activep, nodep, "Should not nest"); VL_RESTORER(m_activep); VL_RESTORER(m_ignoreBlkAndNBlk); + VL_RESTORER(m_inNonCombLogic); m_activep = nodep; AstSenTree* const senTreep = nodep->sensesp(); m_ignoreBlkAndNBlk = senTreep->hasStatic() || senTreep->hasInitial(); + m_inNonCombLogic = senTreep->hasClocked(); iterateChildren(nodep); } void visit(AstNodeProcedure* nodep) override { @@ -854,8 +1073,14 @@ class DelayedVisitor final : public VNVisitor { { VL_RESTORER(m_inSuspendableOrFork); VL_RESTORER(m_procp); + VL_RESTORER(m_ignoreBlkAndNBlk); + VL_RESTORER(m_inNonCombLogic); m_inSuspendableOrFork = nodep->isSuspendable(); m_procp = nodep; + if (m_inSuspendableOrFork) { + m_ignoreBlkAndNBlk = false; + m_inNonCombLogic = true; + } iterateChildren(nodep); } if (m_timingDomains.empty()) return; @@ -1013,8 +1238,8 @@ class DelayedVisitor final : public VNVisitor { nba.nodep = nodep; nba.vscp = vscp; - // Check var usage - checkVarUsage(m_currNbaLhsRefp, true); + // Record write reference + recordWriteRef(m_currNbaLhsRefp, true); iterateChildren(nodep); } @@ -1023,14 +1248,8 @@ class DelayedVisitor final : public VNVisitor { if (nodep == m_currNbaLhsRefp) return; // Only care about write refs if (!nodep->access().isWriteOrRW()) return; - // Check var usage - checkVarUsage(nodep, false); - } - void visit(AstNodeReadWriteMem* nodep) override { - VL_RESTORER(m_ignoreBlkAndNBlk); - // $readmem/$writemem often used in mem models so we suppress BLKANDNBLK warnings - m_ignoreBlkAndNBlk = true; - iterateChildren(nodep); + // Record write reference + recordWriteRef(nodep, false); } void visit(AstNodeFor* nodep) override { // LCOV_EXCL_LINE nodep->v3fatalSrc("For statements should have been converted to while statements"); @@ -1054,6 +1273,7 @@ public: explicit DelayedVisitor(AstNetlist* nodep) { iterate(nodep); } ~DelayedVisitor() override { V3Stats::addStat("NBA, variables using ShadowVar scheme", m_nSchemeShadowVar); + V3Stats::addStat("NBA, variables using ShadowVarMasked scheme", m_nSchemeShadowVarMasked); V3Stats::addStat("NBA, variables using FlagShared scheme", m_nSchemeFlagShared); V3Stats::addStat("NBA, variables using FlagUnique scheme", m_nSchemeFlagUnique); V3Stats::addStat("NBA, variables using ValueQueueWhole scheme", m_nSchemeValueQueuesWhole); diff --git a/src/V3Force.cpp b/src/V3Force.cpp index eeb8e9f9c..ae144dc4f 100644 --- a/src/V3Force.cpp +++ b/src/V3Force.cpp @@ -272,10 +272,8 @@ class ForceConvertVisitor final : public VNVisitor { // it is a variable, and not a net, set the original signal to the forced value, as it // needs to retain the forced value until the next procedural update, which might happen on // a later eval. Luckily we can do all this in a single assignment. - FileLine* const fl_nowarn = new FileLine{flp}; - fl_nowarn->warnOff(V3ErrorCode::BLKANDNBLK, true); AstAssign* const resetRdp - = new AstAssign{fl_nowarn, lhsp->cloneTreePure(false), lhsp->unlinkFrBack()}; + = new AstAssign{flp, lhsp->cloneTreePure(false), lhsp->unlinkFrBack()}; // Replace write refs on the LHS resetRdp->lhsp()->foreach([this](AstNodeVarRef* refp) { if (refp->access() != VAccess::WRITE) return; @@ -283,10 +281,7 @@ class ForceConvertVisitor final : public VNVisitor { AstVarScope* const newVscp = vscp->varp()->isContinuously() ? m_state.getForceComponents(vscp).m_rdVscp : vscp; - // Disable BLKANDNBLK for this reference - FileLine* const flp = new FileLine{refp->fileline()}; - flp->warnOff(V3ErrorCode::BLKANDNBLK, true); - AstVarRef* const newpRefp = new AstVarRef{flp, newVscp, VAccess::WRITE}; + AstVarRef* const newpRefp = new AstVarRef{refp->fileline(), newVscp, VAccess::WRITE}; refp->replaceWith(newpRefp); VL_DO_DANGLING(refp->deleteTree(), refp); }); diff --git a/src/V3OrderGraphBuilder.cpp b/src/V3OrderGraphBuilder.cpp index 7a13de9ce..96b346c05 100644 --- a/src/V3OrderGraphBuilder.cpp +++ b/src/V3OrderGraphBuilder.cpp @@ -254,8 +254,15 @@ class OrderGraphBuilder final : public VNVisitor { // Update VarUsage varscp->user2(varscp->user2() | VU_CON); // Add edges - if (!m_inClocked || m_inPost) { + if (m_inPost) { // Combinational logic + if (!varscp->varp()->ignorePostRead() && m_readTriggersCombLogic(varscp)) { + // Ignore explicit sensitivities + OrderVarVertex* const varVxp = getVarVertex(varscp, VarVertexType::STD); + // Add edge from consumed VarStdVertex -> to consuming LogicVertex + m_graphp->addHardEdge(varVxp, m_logicVxp, WEIGHT_MEDIUM); + } + } else if (!m_inClocked) { // Combinational logic if (m_readTriggersCombLogic(varscp)) { // Ignore explicit sensitivities OrderVarVertex* const varVxp = getVarVertex(varscp, VarVertexType::STD); diff --git a/test_regress/t/t_altera_lpm.v b/test_regress/t/t_altera_lpm.v index 95e16b454..c4419af90 100644 --- a/test_regress/t/t_altera_lpm.v +++ b/test_regress/t/t_altera_lpm.v @@ -46,7 +46,6 @@ //END_MODULE_NAME-------------------------------------------------------------- //See also: https://github.com/twosigma/verilator_support -// verilator lint_off BLKANDNBLK // verilator lint_off COMBDLY // verilator lint_off INITIALDLY // verilator lint_off MULTIDRIVEN diff --git a/test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.out b/test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.out deleted file mode 100644 index e4937d2bd..000000000 --- a/test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.out +++ /dev/null @@ -1,11 +0,0 @@ -%Error-BLKANDNBLK: t/t_assign_on_rhs_of_nonblocking_unsup.v:15:8: Unsupported: Blocked and non-blocking assignments to same variable: 't.x' - 15 | int x; - | ^ - t/t_assign_on_rhs_of_nonblocking_unsup.v:24:18: ... Location of blocking assignment - 24 | y <= (x = 2); - | ^ - t/t_assign_on_rhs_of_nonblocking_unsup.v:21:12: ... Location of nonblocking assignment - 21 | x <= 1; - | ^~ - ... For error description see https://verilator.org/warn/BLKANDNBLK?v=latest -%Error: Exiting due to diff --git a/test_regress/t/t_enum_huge_methods.v b/test_regress/t/t_enum_huge_methods.v index 0c7e1de6d..400836f87 100644 --- a/test_regress/t/t_enum_huge_methods.v +++ b/test_regress/t/t_enum_huge_methods.v @@ -49,21 +49,15 @@ module t (/*AUTOARG*/ end // else if (cyc == 10) begin - /* verilator lint_off BLKANDNBLK */ i_cast <= $cast(e, 60'h1234); - /* verilator lint_on BLKANDNBLK */ end else if (cyc == 11) begin `checkh(i_cast, 0); - /* verilator lint_off BLKANDNBLK */ i_cast <= $cast(e, 60'h1); - /* verilator lint_on BLKANDNBLK */ end else if (cyc == 12) begin `checkh(i_cast, 1); - /* verilator lint_off BLKANDNBLK */ i_cast <= $cast(e, 60'h1234_4567_abcd); - /* verilator lint_on BLKANDNBLK */ end else if (cyc == 13) begin `checkh(i_cast, 1); diff --git a/test_regress/t/t_force.v b/test_regress/t/t_force.v index d6c9fce56..1cf0d1142 100644 --- a/test_regress/t/t_force.v +++ b/test_regress/t/t_force.v @@ -114,7 +114,7 @@ module t(/*AUTOARG*/ release r; end else if (cyc == 45) begin - `checkr(r, 1.25); + `checkr(r, 2.5); end // else if (cyc == 99) begin diff --git a/test_regress/t/t_mem_multidim.v b/test_regress/t/t_mem_multidim.v index a81d5319d..ca1307553 100644 --- a/test_regress/t/t_mem_multidim.v +++ b/test_regress/t/t_mem_multidim.v @@ -12,11 +12,9 @@ module t (/*AUTOARG*/ input clk; // verilator lint_off ASCRANGE - // verilator lint_off BLKANDNBLK // 3 3 4 reg [71:0] memw [2:0][1:3][5:2]; reg [7:0] memn [2:0][1:3][5:2]; - // verilator lint_on BLKANDNBLK integer cyc; initial cyc = 0; reg [63:0] crc; diff --git a/test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.py b/test_regress/t/t_nba_assign_on_rhs.py similarity index 83% rename from test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.py rename to test_regress/t/t_nba_assign_on_rhs.py index 31228c9a7..fc5a55e3f 100755 --- a/test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.py +++ b/test_regress/t/t_nba_assign_on_rhs.py @@ -9,8 +9,10 @@ import vltest_bootstrap -test.scenarios('linter') +test.scenarios('vlt') -test.lint(fails=True, expect_filename=test.golden_filename) +test.compile() + +test.execute() test.passes() diff --git a/test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.v b/test_regress/t/t_nba_assign_on_rhs.v similarity index 100% rename from test_regress/t/t_assign_on_rhs_of_nonblocking_unsup.v rename to test_regress/t/t_nba_assign_on_rhs.v diff --git a/test_regress/t/t_nba_mixed_update_clocked.py b/test_regress/t/t_nba_mixed_update_clocked.py new file mode 100755 index 000000000..5b4c97c80 --- /dev/null +++ b/test_regress/t/t_nba_mixed_update_clocked.py @@ -0,0 +1,24 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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 + +import vltest_bootstrap + +test.scenarios('vlt_all') + +test.compile(verilator_flags2=["--stats", "--unroll-count", "1"]) + +test.execute() + +test.file_grep(test.stats, r'NBA, variables using ShadowVar scheme\s+(\d+)', 1) +test.file_grep(test.stats, r'NBA, variables using ShadowVarMasked scheme\s+(\d+)', 2) +test.file_grep(test.stats, r'NBA, variables using FlagUnique scheme\s+(\d+)', 1) +test.file_grep(test.stats, r'Optimizations, Unrolled Loops\s+(\d+)', 0) +test.file_grep_not(test.stats, r'Warnings, Suppressed BLKANDNBLK') + +test.passes() diff --git a/test_regress/t/t_nba_mixed_update_clocked.v b/test_regress/t/t_nba_mixed_update_clocked.v new file mode 100644 index 000000000..abdf54e54 --- /dev/null +++ b/test_regress/t/t_nba_mixed_update_clocked.v @@ -0,0 +1,59 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +`define stop $stop +`define check(got ,exp) do if ((got) !== (exp)) begin $write("%%Error: %s:%0d: cyc=%0d got='h%x exp='h%x\n", `__FILE__,`__LINE__, cyc, (got), (exp)); `stop; end while(0) + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + reg [31:0] cyc = 0; + + // 'x' has both blocking and non-blocking update, with the blocking + // update in **clocked** logic + reg [1:0] x = 2'b00; + // '{y1, y0}' should have exactly the same value as 'x', at all times + reg y0 = 1'b0; + reg y1 = 1'b0; + // 'z[0]' should equal '{8{x[0]}', 'z[1]' should equal '{8{x[1]}}' + reg [1:0][7:0] z; + // 'pair.a' should equal 'x[0]', 'pair.b' should equal 'x[1]' + struct { + logic a; + logic b; + } pair = '{a: 1'b0, b: 1'b0}; + + always @(posedge clk) begin + $display("cyc = %d (%08x) x[1] = %0d, x[0] = %0d, y1 = %0d, y0 = %0d z[1] = %02x z[1] = %02x pair.a = %0d pair.b = %0d", + cyc, cyc, x[1], x[0], y1, y0, z[1], z[0], pair.a, pair.b); + `check(x[0], cyc[0]); + `check(x[1], cyc[0]); + `check(y0, cyc[0]); + `check(y1, cyc[0]); + `check(z[0], {8{cyc[0]}}); + `check(z[1], {8{cyc[0]}}); + `check(pair.a, cyc[0]); + `check(pair.b, cyc[0]); + x[1] <= ~x[1]; + y1 <= ~y1; + for (int i = 0; i < 8; ++i) z[1][i] <= ~z[1][i]; + pair.b <= ~pair.b; + cyc = cyc + 1; + x[0] = cyc[0]; + y0 = cyc[0]; + for (int i = 0; i < 8; ++i) z[0][i] = cyc[0]; + pair.a = cyc[0]; + if (cyc == 99) begin + $display(x); + $write("*-* All Finished *-*\n"); + $finish; + end + end + +endmodule diff --git a/test_regress/t/t_nba_mixed_update_comb.py b/test_regress/t/t_nba_mixed_update_comb.py new file mode 100755 index 000000000..b0c605c48 --- /dev/null +++ b/test_regress/t/t_nba_mixed_update_comb.py @@ -0,0 +1,24 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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 + +import vltest_bootstrap + +test.scenarios('vlt_all') + +test.compile(verilator_flags2=["--stats", "--unroll-count", "1"]) + +test.execute() + +test.file_grep(test.stats, r'NBA, variables using ShadowVar scheme\s+(\d+)', 1) +test.file_grep(test.stats, r'NBA, variables using ShadowVarMasked scheme\s+(\d+)', 2) +test.file_grep(test.stats, r'NBA, variables using FlagUnique scheme\s+(\d+)', 1) +test.file_grep(test.stats, r'Optimizations, Unrolled Loops\s+(\d+)', 0) +test.file_grep(test.stats, r'Warnings, Suppressed BLKANDNBLK\s+(\d+)', 2) + +test.passes() diff --git a/test_regress/t/t_nba_mixed_update_comb.v b/test_regress/t/t_nba_mixed_update_comb.v new file mode 100644 index 000000000..aa91ac00e --- /dev/null +++ b/test_regress/t/t_nba_mixed_update_comb.v @@ -0,0 +1,72 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +`define stop $stop +`define check(got ,exp) do if ((got) !== (exp)) begin $write("%%Error: %s:%0d: cyc=%0d got='h%x exp='h%x\n", `__FILE__,`__LINE__, cyc, (got), (exp)); `stop; end while(0) + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + + reg [31:0] cyc = 0; + reg [31:0] sameAsCycButCantBeOptimized_0; + reg [31:0] sameAsCycButCantBeOptimized_1; + reg [31:0] sameAsCycButCantBeOptimized_2; + + // 'x' has both blocking and non-blocking update, with the blocking + // update in **combinational** logic + reg [1:0] x = 2'b00; + // '{y1, y0}' should have exactly the same value as 'x', at all times + reg y0 = 1'b0; + reg y1 = 1'b0; + // 'z[0]' should equal '{8{x[0]}', 'z[1]' should equal '{8{x[1]}}' + // verilator lint_off BLKANDNBLK + reg [1:0][7:0] z; + // verilator lint_on BLKANDNBLK + // 'pair.a' should equal 'x[0]', 'pair.b' should equal 'x[1]' + // verilator lint_off BLKANDNBLK + struct { + logic a; + logic b; + } pair = '{a: 1'b0, b: 1'b0}; + // verilator lint_on BLKANDNBLK + + assign x[0] = cyc[0]; + assign y0 = sameAsCycButCantBeOptimized_0[0]; + always_comb begin + for (int i = 0; i < 8; ++i) z[0][i] = sameAsCycButCantBeOptimized_1[0]; + end + assign pair.a = sameAsCycButCantBeOptimized_2[0]; + + always @(posedge clk) begin + $display("cyc = %d (%08x) x[1] = %0d, x[0] = %0d, y1 = %0d, y0 = %0d z[1] = %02x z[1] = %02x", cyc, cyc, x[1], x[0], y1, y0, z[1], z[0]); + `check(x[0], cyc[0]); + `check(x[1], cyc[0]); + `check(y0, cyc[0]); + `check(y1, cyc[0]); + `check(z[0], {8{cyc[0]}}); + `check(z[1], {8{cyc[0]}}); + `check(pair.a, cyc[0]); + `check(pair.b, cyc[0]); + x[1] <= ~x[1]; + y1 <= ~y1; + for (int i = 0; i < 8; ++i) z[1][i] <= ~z[1][i]; + pair.b <= ~pair.b; + cyc = cyc + 1; + sameAsCycButCantBeOptimized_0 = cyc; + sameAsCycButCantBeOptimized_1 = cyc; + sameAsCycButCantBeOptimized_2 = cyc; + if (cyc == 99) begin + $display(x); + $write("*-* All Finished *-*\n"); + $finish; + end + end + +endmodule diff --git a/test_regress/t/t_order_blkandnblk_bad.out b/test_regress/t/t_order_blkandnblk_bad.out index 6c6d38ba8..64452a103 100644 --- a/test_regress/t/t_order_blkandnblk_bad.out +++ b/test_regress/t/t_order_blkandnblk_bad.out @@ -1,11 +1,30 @@ -%Error-BLKANDNBLK: t/t_order_blkandnblk_bad.v:18:21: Unsupported: Blocked and non-blocking assignments to same variable: 't.array' +%Warning-MULTIDRIVEN: t/t_order_blkandnblk_bad.v:33:6: Variable also written to in always_comb (IEEE 1800-2023 9.2.2.2): 'unpacked' + : ... note: In instance 't' + t/t_order_blkandnblk_bad.v:33:6: + 33 | unpacked.b <= unpacked.a; + | ^~~~~~~~ + t/t_order_blkandnblk_bad.v:30:16: ... Location of always_comb write + 30 | always_comb unpacked.a = i; + | ^~~~~~~~ + ... 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. +%Error-BLKANDNBLK: t/t_order_blkandnblk_bad.v:18:21: Unsupported: Blocking and non-blocking assignments to potentially overlapping bits of same packed variable: 't.array' 18 | logic [1:0][3:0] array; | ^~~~~ - t/t_order_blkandnblk_bad.v:20:25: ... Location of blocking assignment + t/t_order_blkandnblk_bad.v:20:25: ... Location of blocking assignment (bits [3:0]) 20 | always_comb array[0] = i; | ^ - t/t_order_blkandnblk_bad.v:23:15: ... Location of nonblocking assignment - 23 | array[1] <= array[0]; - | ^~ + t/t_order_blkandnblk_bad.v:23:6: ... Location of nonblocking assignment (bits [3:0]) + 23 | array[0] <= array[0]; + | ^~~~~ ... For error description see https://verilator.org/warn/BLKANDNBLK?v=latest +%Error-BLKANDNBLK: t/t_order_blkandnblk_bad.v:28:6: Unsupported: Blocking and non-blocking assignments to same non-packed variable: 't.unpacked' + 28 | } unpacked; + | ^~~~~~~~ + t/t_order_blkandnblk_bad.v:30:16: ... Location of blocking assignment + 30 | always_comb unpacked.a = i; + | ^~~~~~~~ + t/t_order_blkandnblk_bad.v:33:6: ... Location of nonblocking assignment + 33 | unpacked.b <= unpacked.a; + | ^~~~~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_order_blkandnblk_bad.v b/test_regress/t/t_order_blkandnblk_bad.v index 9955a06b9..32506aca1 100644 --- a/test_regress/t/t_order_blkandnblk_bad.v +++ b/test_regress/t/t_order_blkandnblk_bad.v @@ -20,8 +20,18 @@ module t (/*AUTOARG*/ always_comb array[0] = i; always @ (posedge clk) - array[1] <= array[0]; + array[0] <= array[0]; - assign o = array[idx]; + struct { + logic [3:0] a; + logic [3:0] b; + } unpacked; + + always_comb unpacked.a = i; + + always @ (posedge clk) + unpacked.b <= unpacked.a; + + assign o = array[idx] + unpacked.a; endmodule diff --git a/test_regress/t/t_order_clkinst.v b/test_regress/t/t_order_clkinst.v index a54fa9397..2b0bf2509 100644 --- a/test_regress/t/t_order_clkinst.v +++ b/test_regress/t/t_order_clkinst.v @@ -14,7 +14,6 @@ module t (/*AUTOARG*/ // verilator lint_off LATCH // verilator lint_off UNOPT // verilator lint_off UNOPTFLAT - // verilator lint_off BLKANDNBLK // verilator lint_off MULTIDRIVEN reg c1_start; initial c1_start = 0; diff --git a/test_regress/t/t_order_comboclkloop.v b/test_regress/t/t_order_comboclkloop.v index 341024aea..52185ca0c 100644 --- a/test_regress/t/t_order_comboclkloop.v +++ b/test_regress/t/t_order_comboclkloop.v @@ -10,7 +10,6 @@ module t (/*AUTOARG*/ ); input clk; - // verilator lint_off BLKANDNBLK // verilator lint_off COMBDLY // verilator lint_off LATCH // verilator lint_off UNOPT diff --git a/test_regress/t/t_param_if_blk.v b/test_regress/t/t_param_if_blk.v index ad660b739..5bacd5eb4 100644 --- a/test_regress/t/t_param_if_blk.v +++ b/test_regress/t/t_param_if_blk.v @@ -92,9 +92,7 @@ module Test output logic [7:0] datao ); - // verilator lint_off BLKANDNBLK logic [7:0] datat; - // verilator lint_on BLKANDNBLK for (genvar i = 0; i < 8; i++) begin if (i%4 != 3) begin