From 041f6603c3cb2c71d42fa3629dcff0eac118b993 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Wed, 9 Oct 2024 11:43:53 +0100 Subject: [PATCH] Make incorrect soft ordering constraint into a hard constraint. (#5520) An ordering constraint between NBA commit blocks ('Post' logic) and the written variable were previously added as soft constraints (cutable edges). However these are required for correctness, so if it ever is cut we will have incorrect simulation results. Change these into hard constraints instead. This necessitates adding a flag on AstVar to ignore special variables constructed during V3Delayed that might otherwise appear as degenerate logic loops. E.g.: if (VdlySet) { VdlySet = 0; // <- This write to VdlySet can and must be ignored LHS = VdlyVal; } No functional change, but you might get an error if this constraint was ever violated. (Theoretically it should never be, as these variables were inserted in a way that does not require violating these constraints ...) --- src/V3AstNodeOther.h | 4 ++++ src/V3Delayed.cpp | 4 +++- src/V3OrderGraphBuilder.cpp | 26 +++++++++++--------------- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index c68217366..8600c7512 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -1847,6 +1847,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_ignorePostWrite : 1; // Ignore writes in 'Post' blocks during ordering void init() { m_ansi = false; @@ -1891,6 +1892,7 @@ class AstVar final : public AstNode { m_isForcedByCode = false; m_isWrittenByDpi = false; m_isWrittenBySuspendable = false; + m_ignorePostWrite = false; m_attrClocker = VVarAttrClocker::CLOCKER_UNKNOWN; } @@ -2046,6 +2048,8 @@ public: void setWrittenByDpi() { m_isWrittenByDpi = true; } bool isWrittenBySuspendable() const { return m_isWrittenBySuspendable; } void setWrittenBySuspendable() { m_isWrittenBySuspendable = true; } + bool ignorePostWrite() const { return m_ignorePostWrite; } + void setIgnorePostWrite() { m_ignorePostWrite = true; } // METHODS void name(const string& name) override { m_name = name; } diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index bf50abb4b..5edee1370 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -510,6 +510,7 @@ class DelayedVisitor final : public VNVisitor { // Create new flag AstVarScope* const flagVscp = createTemp(flp, scopep, "__VdlySet" + baseName, 1); + flagVscp->varp()->setIgnorePostWrite(); // Set the flag at the original NBA nodep->addHereThisAsNext( // new AstAssign{flp, new AstVarRef{flp, flagVscp, VAccess::WRITE}, @@ -543,6 +544,7 @@ class DelayedVisitor final : public VNVisitor { const std::string name = "__VdlyCommitQueue" + vscp->varp()->shortName(); AstVarScope* const queueVscp = createTemp(flp, scopep, name, cqDTypep); queueVscp->varp()->noReset(true); + queueVscp->varp()->setIgnorePostWrite(); vscpInfo.valueQueueKit().vscp = queueVscp; // Create the AstActive for the Post logic AstActive* const activep @@ -556,7 +558,7 @@ class DelayedVisitor final : public VNVisitor { AstCMethodHard* const callp = new AstCMethodHard{flp, new AstVarRef{flp, queueVscp, VAccess::READWRITE}, "commit"}; callp->dtypeSetVoid(); - callp->addPinsp(new AstVarRef{flp, vscp, VAccess::READWRITE}); + callp->addPinsp(new AstVarRef{flp, vscp, VAccess::WRITE}); postp->addStmtsp(callp->makeStmt()); } diff --git a/src/V3OrderGraphBuilder.cpp b/src/V3OrderGraphBuilder.cpp index e7484b86e..d885a399c 100644 --- a/src/V3OrderGraphBuilder.cpp +++ b/src/V3OrderGraphBuilder.cpp @@ -215,24 +215,20 @@ class OrderGraphBuilder final : public VNVisitor { // Update VarUsage varscp->user2(varscp->user2() | VU_GEN); // Add edges for produced variables - if (!m_inClocked || m_inPost) { - // Combinational logic - OrderVarVertex* const varVxp = getVarVertex(varscp, VarVertexType::STD); - // Add edge from producing LogicVertex -> produced VarStdVertex - if (m_inPost) { - m_graphp->addSoftEdge(m_logicVxp, varVxp, WEIGHT_COMBO); - } else { + if (m_inPost) { + if (!varscp->varp()->ignorePostWrite()) { + // Add edge from producing LogicVertex -> produced VarStdVertex + OrderVarVertex* const varVxp = getVarVertex(varscp, VarVertexType::STD); m_graphp->addHardEdge(m_logicVxp, varVxp, WEIGHT_NORMAL); } - + OrderVarVertex* const postVxp = getVarVertex(varscp, VarVertexType::POST); + // Add edge from produced VarPostVertex -> to producing LogicVertex + m_graphp->addHardEdge(postVxp, m_logicVxp, WEIGHT_POST); + } else if (!m_inClocked) { // Combinational logic + // Add edge from producing LogicVertex -> produced VarStdVertex + OrderVarVertex* const varVxp = getVarVertex(varscp, VarVertexType::STD); + m_graphp->addHardEdge(m_logicVxp, varVxp, WEIGHT_NORMAL); // Add edge from produced VarPostVertex -> to producing LogicVertex - - // For m_inPost: - // Add edge consumed_var_POST->logic_vertex - // This prevents a consumer of the "early" value to be scheduled - // after we've changed to the next-cycle value - // ALWAYS do it: - // There maybe a wire a=b; between the two blocks OrderVarVertex* const postVxp = getVarVertex(varscp, VarVertexType::POST); m_graphp->addHardEdge(postVxp, m_logicVxp, WEIGHT_POST); } else if (m_inPre) { // AstAssignPre