Address disable handling review feedback (#7856)

src/V3Ast.h

@@ -730,6 +730,7 @@ public:
 
     // ACCESSORS for specific types
     // Alas these can't be virtual or they break when passed a nullptr
+    bool isDisableQueuePushSelfStmt();
     inline bool isClassHandleValue() const;
     inline bool isNull() const;
     inline bool isZero() const;

src/V3AstNodes.cpp

@@ -108,6 +108,21 @@ int AstNodeSel::bitConst() const {
     return (constp ? constp->toSInt() : 0);
 }
 
+bool AstNode::isDisableQueuePushSelfStmt() {
+    // Detect LinkJump-generated registration:
+    // __VprocessQueue_*.push_back(std::process::self())
+    AstStmtExpr* const stmtExprp = VN_CAST(this, StmtExpr);
+    if (!stmtExprp) return false;
+    AstCMethodHard* const methodp = VN_CAST(stmtExprp->exprp(), CMethodHard);
+    if (!methodp || methodp->name() != "push_back") return false;
+    AstNode* const basep = AstArraySel::baseFromp(methodp->fromp(), false);
+    if (AstVarRef* const refp = VN_CAST(basep, VarRef)) return refp->varp()->processQueue();
+    if (AstMemberSel* const selp = VN_CAST(basep, MemberSel)) {
+        return selp->varp() && selp->varp()->processQueue();
+    }
+    return false;
+}
+
 void AstNodeStmt::dump(std::ostream& str) const { this->AstNode::dump(str); }
 void AstNodeStmt::dumpJson(std::ostream& str) const { dumpJsonGen(str); }
 

src/V3Fork.cpp

@@ -593,40 +593,18 @@ class ForkVisitor final : public VNVisitor {
         const AstConst* const constp = VN_CAST(delayp->lhsp(), Const);
         return constp && (constp->toUQuad() == std::numeric_limits<uint64_t>::max());
     }
-    static bool isDisableProcessQueueExpr(const AstNodeExpr* const nodep) {
+    static bool isDisableQueuePushSelfPrefix(AstNode* nodep) {
-        const AstNode* const basep
+        while (AstJumpBlock* const jumpBlockp = VN_CAST(nodep, JumpBlock)) {
-            = AstArraySel::baseFromp(const_cast<AstNodeExpr*>(nodep), false);
-        if (const AstVarRef* const refp = VN_CAST(basep, VarRef))
-            return refp->varp()->processQueue();
-        if (const AstMemberSel* const selp = VN_CAST(basep, MemberSel)) {
-            return selp->varp() && selp->varp()->processQueue();
-        }
-        return false;
-    }
-    static bool isDisableQueuePushSelfStmt(const AstNode* const nodep) {
-        // Detect LinkJump-generated registration:
-        // __VprocessQueue_*.push_back(std::process::self())
-        const AstStmtExpr* const stmtExprp = VN_CAST(nodep, StmtExpr);
-        if (!stmtExprp) return false;
-        const AstCMethodHard* const methodp = VN_CAST(stmtExprp->exprp(), CMethodHard);
-        if (!methodp || methodp->name() != "push_back") return false;
-        return isDisableProcessQueueExpr(methodp->fromp());
-    }
-    static const AstNode* unwrapLeadingJumpBlocks(const AstNode* nodep) {
-        while (const AstJumpBlock* const jumpBlockp = VN_CAST(nodep, JumpBlock)) {
             nodep = jumpBlockp->stmtsp();
         }
-        return nodep;
+        return nodep && nodep->isDisableQueuePushSelfStmt();
-    }
-    static bool isDisableQueuePushSelfPrefix(const AstNode* const nodep) {
-        return isDisableQueuePushSelfStmt(unwrapLeadingJumpBlocks(nodep));
     }
     template <typename T_Owner>
     static bool insertForkSentinelAfterDisableQueuePushes(T_Owner* const ownerp,
                                                           AstNode* const firstStmtp,
                                                           AstNode* const delayp) {
         AstNode* insertBeforep = firstStmtp;
-        while (insertBeforep && isDisableQueuePushSelfStmt(insertBeforep)) {
+        while (insertBeforep && insertBeforep->isDisableQueuePushSelfStmt()) {
             insertBeforep = insertBeforep->nextp();
         }
         if (AstJumpBlock* const jumpBlockp = VN_CAST(insertBeforep, JumpBlock)) {
@@ -654,9 +632,8 @@ class ForkVisitor final : public VNVisitor {
             = insertForkSentinelAfterDisableQueuePushes(beginp, afterSentinelp, delayp);
         UASSERT_OBJ(moved, beginp, "Failed to move fork sentinel after disable queue pushes");
     }
-    static bool forkIsDisableable(const AstFork* const nodep) {
+    static bool forkIsDisableable(AstFork* const nodep) {
-        for (const AstBegin* itemp = nodep->forksp(); itemp;
+        for (AstBegin* itemp = nodep->forksp(); itemp; itemp = VN_AS(itemp->nextp(), Begin)) {
-             itemp = VN_AS(itemp->nextp(), Begin)) {
             if (isDisableQueuePushSelfPrefix(itemp->stmtsp())) return true;
         }
         return false;
@@ -680,7 +657,8 @@ class ForkVisitor final : public VNVisitor {
         // start executing until the parent process is blocked or terminates. Because join and
         // join_any block the parent process, deferring branch start with a synthetic #0 delay is
         // normally only needed for join_none. A fork that can be disabled by name needs the same
-        // deferral for every join type.
+        // deferral for every join type so all branches register their processes before any branch
+        // body can disable the block.
         if (nodep->joinType().joinNone() || forkIsDisableable(nodep)) {
             UINFO(9, "Adding fork branch start sentinels " << nodep);
             FileLine* fl = nodep->fileline();

src/V3LinkDot.cpp

@@ -6209,7 +6209,7 @@ class LinkDotResolveVisitor final : public VNVisitor {
                 nodep->targetRefp()->unlinkFrBack()->deleteTree();
                 if (!hasPartSelect) {
                     const string instancePath = targetInstancePath(nodep->targetp(), targetPath);
-                    if (instancePath != "") {
+                    if (!instancePath.empty()) {
                         // Keep only the instance prefix so V3LinkJump can reference the selected
                         // instance's process queue without reparsing the disable target.
                         nodep->targetRefp(

src/V3LinkJump.cpp

@@ -607,8 +607,8 @@ class LinkJumpVisitor final : public VNVisitor {
                     // If disable executes inside a fork branch of this named block, jump to the
                     // end of that branch to prevent statements after disable from executing.
                     AstBegin* const branchp = innerForkBranchp(beginp);
-                    AstBegin* const jumpTargetp = branchp ? branchp : m_beginDisableBegins[beginp];
+                    AstBegin* const jumpTargetp
-                    UASSERT_OBJ(jumpTargetp, nodep, "Missing disable jump target");
+                        = branchp ? branchp : m_beginDisableBegins.at(beginp);
                     addJumpAfterKill(killStmtp, jumpTargetp);
                 }
             } else {

src/V3Timing.cpp

@@ -749,31 +749,19 @@ class TimingControlVisitor final : public VNVisitor {
         addDebugInfo(donep);
         beginp->addStmtsp(donep->makeStmt());
     }
-    static bool isDisableProcessQueueExpr(const AstNodeExpr* const nodep) {
+    static bool hasDisableQueuePushSelfPrefix(AstBegin* const beginp) {
-        const AstNode* const basep
-            = AstArraySel::baseFromp(const_cast<AstNodeExpr*>(nodep), false);
-        if (const AstVarRef* const refp = VN_CAST(basep, VarRef))
-            return refp->varp()->processQueue();
-        if (const AstMemberSel* const selp = VN_CAST(basep, MemberSel)) {
-            return selp->varp() && selp->varp()->processQueue();
-        }
-        return false;
-    }
-    static bool hasDisableQueuePushSelfPrefix(const AstBegin* const beginp) {
         // LinkJump prepends disable-by-name registration as:
         //   __VprocessQueue_*.push_back(std::process::self())
-        // By this pass V3LiftExpr has lifted the std::process::self() argument into a preceding
+        // V3LiftExpr lifts std::process::self() into an assignment to a temporary, then V3Task
-        // temporary (and left a leading comment), so the push_back is no longer necessarily the
+        // lowers that assignment's function call into a leading comment and AstStmtExpr. Thus the
-        // first statement. Scan across the leading registration statements for it, stopping at the
+        // push_back is no longer necessarily the first statement. Scan across this generated
-        // fork-start sentinel or the branch body. The branch's own registration is at the front;
+        // prefix for it, stopping at the fork-start sentinel or the branch body. Registrations for
-        // registrations for nested forks live in sub-blocks and so are not in this statement list.
+        // nested forks live in sub-blocks and so are not in this statement list.
-        for (const AstNode* stmtp = beginp->stmtsp(); stmtp; stmtp = stmtp->nextp()) {
+        for (AstNode* stmtp = beginp->stmtsp(); stmtp; stmtp = stmtp->nextp()) {
             if (VN_IS(stmtp, Comment)) continue;
-            const AstStmtExpr* const stmtExprp = VN_CAST(stmtp, StmtExpr);
+            AstStmtExpr* const stmtExprp = VN_CAST(stmtp, StmtExpr);
             if (!stmtExprp) break;
-            const AstCMethodHard* const methodp = VN_CAST(stmtExprp->exprp(), CMethodHard);
+            if (stmtExprp->isDisableQueuePushSelfStmt()) return true;
-            if (!methodp || methodp->name() != "push_back") continue;
-            if (isDisableProcessQueueExpr(methodp->fromp())) return true;
         }
         return false;
     }