Improve V3MergeCond by reordering statements (#3125)

V3MergeCond merges consecutive conditional `_ = cond ? _ : _` and
`if (cond) ...` statements. This patch adds an analysis and ordering
phase that moves statements with identical conditions closer to each
other, in order to enable more merging opportunities. This in turn
eliminates a lot of repeated conditionals which reduced dynamic branch
count and branch misprediction rate. Observed 6.5% improvement on
multi-threaded large designs, at the cost of less than 2% increase in
Verilation speed.

Changes

@@ -17,6 +17,7 @@ Verilator 4.223 devel
 * Add assert when VerilatedContext is mis-deleted (#3121). [Rupert Swarbrick]
 * Define VM_TRACE_VCD when tracing in VCD format. [Geza Lore, Shunyao CAD]
 * Support non-ANSI interface port declarations (#3439). [Geza Lore, Shunyao CAD]
+* Improve conditional merging optimization (#3125). [Geza Lore, Shunyao CAD]
 * Fix hang with large case statement optimization (#3405). [Mike Urbach]
 * Fix 'with' operator with type casting (#3387). [xiak95]
 * Fix incorrect conditional merging (#3409). [Raynard Qiao]

src/V3AstUserAllocator.h

@@ -106,7 +106,7 @@ public:
     }
 
     // Get a reference to the user data
-    T_Data& operator()(const T_Node* nodep) {
+    T_Data& operator()(const T_Node* nodep) const {
         T_Data* const userp = getUserp(nodep);
         UASSERT_OBJ(userp, nodep, "Missing User data on const AstNode");
         return *userp;

src/V3MergeCond.cpp

@@ -42,6 +42,34 @@
 //
 //  Also merges consecutive AstNodeIf statements with the same condition.
 //
+//  Because this optimization has notable performance impact, we go further
+//  and perform code motion to try to move mergeable conditionals next to each
+//  other, which in turn enable us to merge more conditionals. To do this, we
+//  perform an analysis pass, followed by an optimization pass on the whole
+//  AstCFunc we are optimizing.
+//
+//  The analysis pass gathers, for each statement in the tree, the information
+//  relevant for determining whether two statements can be swapped, and some
+//  other additional information that is useful during optimization.
+//
+//  The optimization pass tries to move conditionals near each other, first by
+//  trying to move a conditional node backwards in the list, so it becomes the
+//  direct successor of another earlier conditional with the same condition.
+//  If this is not possible due to variable interference, then we additionally
+//  try to pull earlier conditionals with the same condition closer forward to
+//  be the immediate predecessor of the conditional node. We limit maximum
+//  distance a node can travel to an empirically chosen but otherwise arbitrary
+//  constant. This limits worst case complexity to be O(n) rather than O(n^2).
+//  The worst case complexity manifests when N/2 conditionals, all with unique
+//  conditions are succeeded by N/2 conditionals with the same unique
+//  conditions, such that each unique condition is used by exactly 2
+//  conditionals. In this case N/2 all nodes need to travel approx N/2 distance.
+//  Limiting the distance bounds the latter, hence limiting complexity.
+//
+//  Once the analysis and optimization passes have been applied to the whole
+//  function, any merged conditionals will then undergo the same analysis,
+//  optimization, and merging again in their individual branches.
+//
 //*************************************************************************
 
 #include "config_build.h"
@@ -51,71 +79,364 @@
 #include "V3MergeCond.h"
 #include "V3Stats.h"
 #include "V3Ast.h"
+#include "V3AstUserAllocator.h"
+#include "V3Hasher.h"
+#include "V3DupFinder.h"
+
+#include <queue>
+#include <set>
+
+namespace {
 
 //######################################################################
+// Utilities
 
-enum class Mergeable {
+// This function extracts the Cond node from the RHS of an assignment,
-    YES,  // Tree can be merged
+// if there is one and it is in a supported position, which are:
-    NO_COND_ASSIGN,  // Tree cannot be merged because it contains an assignment to a condition
+// - RHS is the Cond
-    NO_IMPURE  // Tree cannot be merged because it contains an impure node
+// - RHS is And(Const, Cond). This And is inserted often by V3Clean.
+AstNodeCond* extractCondFromRhs(AstNode* rhsp) {
+    if (AstNodeCond* const condp = VN_CAST(rhsp, NodeCond)) {
+        return condp;
+    } else if (const AstAnd* const andp = VN_CAST(rhsp, And)) {
+        if (AstNodeCond* const condp = VN_CAST(andp->rhsp(), NodeCond)) {
+            if (VN_IS(andp->lhsp(), Const)) return condp;
+        }
+    }
+    return nullptr;
+}
+
+// Predicate to check if two sets are disjoint. This is stable, as we only need
+// to determine if the sets contain a shared element, which is a boolean
+// property. It is also efficient as we use sorted sets, and therefore can
+// enumerate elements in order (what the ordering is, is unimportant), meaning
+// the worst case complexity is O(size of smaller set).
+bool areDisjoint(const std::set<const AstVar*>& a, const std::set<const AstVar*>& b) {
+    if (a.empty() || b.empty()) return true;
+    const auto endA = a.end();
+    const auto endB = b.end();
+    auto itA = a.begin();
+    auto itB = b.begin();
+    while (true) {
+        if (*itA == *itB) return false;
+        if (std::less<const AstVar*>{}(*itA, *itB)) {
+            itA = std::lower_bound(++itA, endA, *itB);
+            if (itA == endA) return true;
+        } else {
+            itB = std::lower_bound(++itB, endB, *itA);
+            if (itB == endB) return true;
+        }
+    }
+}
+
+//######################################################################
+// Structure containing information required for code motion/merging
+
+struct StmtProperties {
+    AstNode* m_condp = nullptr;  // The condition expression, if a conditional node
+    std::set<const AstVar*> m_rdVars;  // Variables read by this statement
+    std::set<const AstVar*> m_wrVars;  // Variables writen by this statement
+    bool m_isFence = false;  // Nothing should move across this statement, nor should it be merged
+    AstNodeStmt* m_prevWithSameCondp = nullptr;  // Previous node in same list, with same condition
+    bool writesConditionVar() const {
+        // This relies on MarkVarsVisitor having been called on the condition node
+        for (const AstVar* const varp : m_wrVars) {
+            if (varp->user1()) return true;
+        }
+        return false;
+    }
 };
 
-class CheckMergeableVisitor final : public VNVisitor {
+// We store the statement properties in user3 via AstUser3Allocator
-private:
+using StmtPropertiesAllocator = AstUser3Allocator<AstNodeStmt, StmtProperties>;
-    // STATE
-    bool m_condAssign = false;  // Does this tree contain an assignment to a condition variable??
-    bool m_impure = false;  // Does this tree contain an impure node?
 
-    // METHODS
+//######################################################################
-    VL_DEBUG_FUNC;  // Declare debug()
+// Code motion analysis and implementation
 
-    // VISITORS
+// Pure analysis visitor that build the StmtProperties for each statement in the given
-    virtual void visit(AstNode* nodep) override {
+// AstNode list (following AstNode::nextp())
-        if (m_impure) return;
+class CodeMotionAnalysisVisitor final : public VNVisitor {
-        // Clear if node is impure
+    // NODE STATE
-        if (!nodep->isPure()) {
+    // AstNodeStmt::user3   -> StmtProperties (accessed via m_stmtProperties, managed externally,
-            UINFO(9, "Not mergeable due to impure node" << nodep << endl);
+    //                         see MergeCondVisitor::process)
-            m_impure = true;
+    // AstNode::user4       -> Used by V3Hasher
-            return;
+    // AstNode::user5       -> AstNode*: Set on a condition node, points to the last conditional
+    //                         with that condition so far encountered in the same AstNode list
+
+    VNUser5InUse m_user5InUse;
+
+    StmtPropertiesAllocator& m_stmtProperties;
+
+    // MEMBERS
+    V3Hasher m_hasher;  // Used by V3DupFinder
+    // Stack of a V3DupFinder used for finding identical condition expressions within one
+    // statement list.
+    std::vector<V3DupFinder> m_stack;
+    StmtProperties* m_propsp = nullptr;  // StmtProperties structure of current AstNodeStmt
+
+    // Extract condition expression from a megeable conditional statement, if any
+    static AstNode* extractCondition(const AstNodeStmt* nodep) {
+        AstNode* conditionp = nullptr;
+        if (const AstNodeAssign* const assignp = VN_CAST(nodep, NodeAssign)) {
+            if (AstNodeCond* const conditionalp = extractCondFromRhs(assignp->rhsp())) {
+                conditionp = conditionalp->condp();
+            }
+        } else if (const AstNodeIf* const ifp = VN_CAST(nodep, NodeIf)) {
+            conditionp = ifp->condp();
         }
+        while (AstCCast* const castp = VN_CAST(conditionp, CCast)) conditionp = castp->lhsp();
+        return conditionp;
+    }
+
+    void analyzeStmt(AstNodeStmt* nodep, bool tryCondMatch) {
+        VL_RESTORER(m_propsp);
+        // Keep hold of props of enclosing statement
+        StmtProperties* const outerPropsp = m_propsp;
+        // Grab the props of this statement
+        m_propsp = &m_stmtProperties(nodep);
+
+        // Extract condition from statement
+        if (AstNode* const condp = extractCondition(nodep)) {
+            // Remember condition node. We always need this as it is used in the later
+            // traversal.
+            m_propsp->m_condp = condp;
+            // If this is a conditional statement, try to find an earlier one with the same
+            // condition in the same list (unless we have been told not to bother because we know
+            // this node is in a singleton list).
+            if (tryCondMatch) {
+                // Grab the duplicate finder of this list
+                V3DupFinder& dupFinder = m_stack.back();
+                // Find a duplicate condition
+                const V3DupFinder::iterator& dit = dupFinder.findDuplicate(condp);
+                if (dit == dupFinder.end()) {
+                    // First time seeing this condition in the current list
+                    dupFinder.insert(condp);
+                    // Remember last statement with this condition (which is this statement)
+                    condp->user5p(nodep);
+                } else {
+                    // Seen a conditional with the same condition earlier in the current list
+                    AstNode* const firstp = dit->second;
+                    // Add to properties for easy retrieval during optimization
+                    m_propsp->m_prevWithSameCondp = static_cast<AstNodeStmt*>(firstp->user5p());
+                    // Remember last statement with this condition (which is this statement)
+                    firstp->user5p(nodep);
+                }
+            }
+        }
+
+        // Analyse this statement
+        analyzeNode(nodep);
+
+        // If there is an enclosing statement, propagate properties upwards
+        if (outerPropsp) {
+            // Add all rd/wr vars to outer statement
+            outerPropsp->m_rdVars.insert(m_propsp->m_rdVars.cbegin(), m_propsp->m_rdVars.cend());
+            outerPropsp->m_wrVars.insert(m_propsp->m_wrVars.cbegin(), m_propsp->m_wrVars.cend());
+            // If this statement is impure, the enclosing statement is also impure
+            if (m_propsp->m_isFence) outerPropsp->m_isFence = true;
+        }
+    }
+
+    void analyzeVarRef(AstVarRef* nodep) {
+        const VAccess access = nodep->access();
+        AstVar* const varp = nodep->varp();
+        // Gather read and written variables
+        if (access.isReadOrRW()) m_propsp->m_rdVars.insert(varp);
+        if (access.isWriteOrRW()) m_propsp->m_wrVars.insert(varp);
+    }
+
+    void analyzeNode(AstNode* nodep) {
+        // If an impure node under a statement, mark that statement as impure
+        if (m_propsp && !nodep->isPure()) m_propsp->m_isFence = true;
+        // Analyze children
         iterateChildrenConst(nodep);
     }
-    virtual void visit(AstVarRef* nodep) override {
+
-        if (m_impure || m_condAssign) return;
+    // VISITORS
-        // Clear if it's an LValue referencing a marked variable
+    void visit(AstNode* nodep) override {
-        if (nodep->access().isWriteOrRW() && nodep->varp()->user1()) {
+        // Push a new stack entry at the start of a list, but only if the list is not a
-            UINFO(9, "Not mergeable due assignment to condition" << nodep << endl);
+        // single element (this saves a lot of allocations in expressions)
-            m_condAssign = true;
+        bool singletonListStart = false;
+        if (nodep->backp()->nextp() != nodep) {  // If at head of list
+            singletonListStart = nodep->nextp() == nullptr;
+            if (!singletonListStart) m_stack.emplace_back(m_hasher);
         }
+
+        // Analyse node
+        if (AstNodeStmt* const stmtp = VN_CAST(nodep, NodeStmt)) {
+            analyzeStmt(stmtp, /*tryCondMatch:*/ !singletonListStart);
+        } else if (AstVarRef* const vrefp = VN_CAST(nodep, VarRef)) {
+            analyzeVarRef(vrefp);
+        } else {
+            analyzeNode(nodep);
+        }
+
+        // Pop the stack at the end of a list
+        if (!singletonListStart && !nodep->nextp()) m_stack.pop_back();
+    }
+
+    // CONSTRUCTOR
+    CodeMotionAnalysisVisitor(AstNode* nodep, StmtPropertiesAllocator& stmtProperties)
+        : m_stmtProperties(stmtProperties) {
+        iterateAndNextConstNull(nodep);
     }
 
 public:
-    CheckMergeableVisitor() = default;
+    // Analyse the statement list starting at nodep, filling in stmtProperties.
-
+    static void analyze(AstNode* nodep, StmtPropertiesAllocator& stmtProperties) {
-    // Return false if this node should not be merged at all because:
+        CodeMotionAnalysisVisitor{nodep, stmtProperties};
-    // - It contains an impure expression
-    // - It contains an LValue referencing the condition
-    Mergeable operator()(const AstNode* node) {
-        m_condAssign = false;
-        m_impure = false;
-        iterateChildrenConst(const_cast<AstNode*>(node));
-        if (m_impure) {  // Impure is stronger than cond assign
-            return Mergeable::NO_IMPURE;
-        } else if (m_condAssign) {
-            return Mergeable::NO_COND_ASSIGN;
-        } else {
-            return Mergeable::YES;
-        }
     }
 };
 
+class CodeMotionOptimizeVisitor final : public VNVisitor {
+    // Do not move a node more than this many statements.
+    // This bounds complexity at O(N), rather than O(N^2).
+    static constexpr unsigned MAX_DISTANCE = 500;
+
+    // NODE STATE
+    // AstNodeStmt::user3   -> StmtProperties (accessed via m_stmtProperties, managed externally,
+    //                         see MergeCondVisitor::process)
+    // AstNodeStmt::user4   -> bool: Already processed this node
+
+    VNUser4InUse m_user4InUse;
+
+    const StmtPropertiesAllocator& m_stmtProperties;
+
+    // MEMBERS
+
+    // Predicate that checks if the order of two statements can be swapped
+    bool areSwappable(const AstNodeStmt* ap, const AstNodeStmt* bp) const {
+        const StmtProperties& aProps = m_stmtProperties(ap);
+        const StmtProperties& bProps = m_stmtProperties(bp);
+        // Don't move across fences
+        if (aProps.m_isFence) return false;
+        if (bProps.m_isFence) return false;
+        // If either statement writes a variable that the other reads, they are not swappable
+        if (!areDisjoint(aProps.m_rdVars, bProps.m_wrVars)) return false;
+        if (!areDisjoint(bProps.m_rdVars, aProps.m_wrVars)) return false;
+        // If they both write to the same variable, they are not swappable
+        if (!areDisjoint(aProps.m_wrVars, bProps.m_wrVars)) return false;
+        // Otherwise good to go
+        return true;
+    }
+
+    // VISITORS
+    void visit(AstNodeStmt* nodep) override {
+        // Process only on first encounter
+        if (nodep->user4SetOnce()) return;
+        // First re-order children
+        iterateChildren(nodep);
+        // Grab hold of previous node with same condition
+        AstNodeStmt* prevp = m_stmtProperties(nodep).m_prevWithSameCondp;
+        // If no previous node with same condition, we are done
+        if (!prevp) return;
+#ifdef VL_DEBUG
+        {  // Sanity check, only in debug build, otherwise expensive
+            const AstNode* currp = prevp;
+            while (currp && currp != nodep) currp = currp->nextp();
+            UASSERT_OBJ(currp, nodep, "Predecessor not in same list as " << currp);
+        }
+#endif
+        // Otherwise try to move this node backwards, as close as we can to the previous node
+        // with the same condition
+        if (AstNodeStmt* predp = VN_CAST(nodep->backp(), NodeStmt)) {
+            // 'predp' is the newly computed predecessor node of 'nodep', which is initially
+            // (without movement) the 'backp' of the node.
+            for (unsigned i = MAX_DISTANCE; i; --i) {
+                // If the predecessor is the previous node with the same condition, job done
+                if (predp == prevp) break;
+                // Don't move past a non-statement (e.g.: AstVar), or end of list
+                AstNodeStmt* const backp = VN_CAST(predp->backp(), NodeStmt);
+                if (!backp) break;
+                // Don't swap statements if doing so would change program semantics
+                if (!areSwappable(predp, nodep)) break;
+                // Otherwise move 'nodep' back
+                predp = backp;
+            }
+
+            // If we decided that 'nodep' should be moved back
+            if (nodep->backp() != predp) {
+                // Move the current node to directly follow the computed predecessor
+                nodep->unlinkFrBack();
+                predp->addNextHere(nodep);
+                // If the predecessor is the previous node with the same condition, job done
+                if (predp == prevp) return;
+            }
+        }
+        // If we reach here, it means we were unable to move the current node all the way back
+        // such that it immediately follows the previous statement with the same condition. Now
+        // try to move all previous statements with the same condition forward, in the hope of
+        // compacting the list further.
+        for (AstNodeStmt* currp = nodep; prevp;
+             currp = prevp, prevp = m_stmtProperties(currp).m_prevWithSameCondp) {
+            // Move prevp (previous statement with same condition) towards currp
+            if (AstNodeStmt* succp = VN_CAST(prevp->nextp(), NodeStmt)) {
+                // 'succp' is the newly computed successor node of 'prevp', which is initially
+                // (without movement) the 'nextp' of the node.
+                for (unsigned i = MAX_DISTANCE; --i;) {
+                    // If the successor of the previous statement with same condition is the
+                    // target node, we are done with this predecessor
+                    if (succp == currp) break;
+                    // Don't move past a non-statement (e.g.: AstVar), or end of list
+                    AstNodeStmt* const nextp = VN_CAST(succp->nextp(), NodeStmt);
+                    if (!nextp) break;
+                    // Don't swap statements if doing so would change program semantics
+                    if (!areSwappable(prevp, succp)) break;
+                    // Otherwise move further forward
+                    succp = nextp;
+                }
+
+                // If we decided that 'prevp' should be moved forward
+                if (prevp->nextp() != succp) {
+                    // Move the current node to directly before the computed successor
+                    prevp->unlinkFrBack();
+                    succp->addHereThisAsNext(prevp);
+                }
+            }
+        }
+    }
+
+    void visit(AstNode* nodep) override {}  // Ignore all non-statements
+
+    // CONSTRUCTOR
+    CodeMotionOptimizeVisitor(AstNode* nodep, const StmtPropertiesAllocator& stmtProperties)
+        : m_stmtProperties(stmtProperties) {
+        // We assert the given node is at the head of the list otherwise we might move a node
+        // before the given node. This is easy to fix in the above iteration with a check on a
+        // boundary node we should not move past, if we ever need to do so.
+        // Note: we will do iterateAndNextNull which requires nodep->backp() != nullptr anyway
+        UASSERT_OBJ(nodep->backp()->nextp() != nodep, nodep, "Must be at head of list");
+        // Optimize the list
+        iterateAndNextNull(nodep);
+    }
+
+public:
+    // Given an AstNode list (held via AstNode::nextp()), move conditional statements as close
+    // together as possible
+    static AstNode* optimize(AstNode* nodep, const StmtPropertiesAllocator& stmtProperties) {
+        CodeMotionOptimizeVisitor{nodep, stmtProperties};
+        // It is possible for the head of the list to be moved later such that it is no longer
+        // in head position. If so, rewind the list and return the new head.
+        while (nodep->backp()->nextp() == nodep) nodep = nodep->backp();
+        return nodep;
+    }
+};
+
+//######################################################################
+// Conditional merging
+
 class MergeCondVisitor final : public VNVisitor {
 private:
     // NODE STATE
-    // AstVar::user1        -> Flag set for variables referenced by m_mgCondp
+    // AstVar::user1        -> bool: Set for variables referenced by m_mgCondp
-    // AstNode::user2       -> Flag marking node as included in merge because cheap to duplicate
+    //                         (Only below MergeCondVisitor::process).
-    const VNUser1InUse m_user1InUse;
+    // AstNode::user2       -> bool: Marking node as included in merge because cheap to
-    const VNUser2InUse m_user2InUse;
+    //                         duplicate
+    //                         (Only below MergeCondVisitor::process).
+    // AstNodeStmt::user3   -> StmtProperties
+    //                         (Only below MergeCondVisitor::process).
+    // AstNode::user4       -> See CodeMotionAnalysisVisitor/CodeMotionOptimizeVisitor
+    // AstNode::user5       -> See CodeMotionAnalysisVisitor
 
     // STATE
     VDouble0 m_statMerges;  // Statistic tracking
@@ -128,24 +449,84 @@ private:
     const AstNode* m_mgNextp = nullptr;  // Next node in list being examined
     uint32_t m_listLenght = 0;  // Length of current list
 
-    CheckMergeableVisitor m_checkMergeable;  // Sub visitor for encapsulation & speed
+    std::queue<AstNode*>* m_workQueuep = nullptr;  // Node lists (via AstNode::nextp()) to merge
+    // Statement properties for code motion and merging
+    StmtPropertiesAllocator* m_stmtPropertiesp = nullptr;
 
     // METHODS
     VL_DEBUG_FUNC;  // Declare debug()
 
-    // This function extracts the Cond node from the RHS, if there is one and
+    // Function that processes a whole sub-tree
-    // it is in a supported position, which are:
+    void process(AstNode* nodep) {
-    // - RHS is the Cond
+        // Set up work queue
-    // - RHS is And(Const, Cond). This And is inserted often by V3Clean.
+        std::queue<AstNode*> workQueue;
-    static AstNodeCond* extractCond(AstNode* rhsp) {
+        m_workQueuep = &workQueue;
-        if (AstNodeCond* const condp = VN_CAST(rhsp, NodeCond)) {
+        m_workQueuep->push(nodep);
-            return condp;
+
-        } else if (const AstAnd* const andp = VN_CAST(rhsp, And)) {
+        do {
-            if (AstNodeCond* const condp = VN_CAST(andp->rhsp(), NodeCond)) {
+            // Set up user* for this iteration
-                if (VN_IS(andp->lhsp(), Const)) return condp;
+            const VNUser1InUse user1InUse;
-            }
+            const VNUser2InUse user2InUse;
+            const VNUser3InUse user3InUse;
+            // Statement properties only preserved for this iteration,
+            // then memory is released immediately.
+            StmtPropertiesAllocator stmtProperties;
+            m_stmtPropertiesp = &stmtProperties;
+
+            // Pop off current work item
+            AstNode* currp = m_workQueuep->front();
+            m_workQueuep->pop();
+
+            // Analyse sub-tree list for code motion
+            CodeMotionAnalysisVisitor::analyze(currp, stmtProperties);
+            // Perform the code motion within the whole sub-tree list
+            currp = CodeMotionOptimizeVisitor::optimize(currp, stmtProperties);
+
+            // Merge conditionals in the whole sub-tree list (this might create new work items)
+            iterateAndNextNull(currp);
+
+            // Close pending merge, if there is one at the end of the whole sub-tree list
+            if (m_mgFirstp) mergeEnd();
+        } while (!m_workQueuep->empty());
+    }
+
+    // Skip past AstArraySel and AstWordSel with const index
+    static AstNode* skipConstSels(AstNode* nodep) {
+        while (const AstArraySel* const aselp = VN_CAST(nodep, ArraySel)) {
+            // ArraySel index is not constant, so might be expensive
+            if (!VN_IS(aselp->bitp(), Const)) return nodep;
+            nodep = aselp->fromp();
         }
-        return nullptr;
+        while (const AstWordSel* const wselp = VN_CAST(nodep, WordSel)) {
+            // WordSel index is not constant, so might be expensive
+            if (!VN_IS(wselp->bitp(), Const)) return nodep;
+            nodep = wselp->fromp();
+        }
+        return nodep;
+    }
+
+    // Check if this node is cheap enough that duplicating it in two branches of an
+    // AstIf is not likely to cause a performance degradation.
+    static bool isCheapNode(AstNode* nodep) {
+        // Comments are cheap
+        if (VN_IS(nodep, Comment)) return true;
+        // So are some assignments
+        if (const AstNodeAssign* const assignp = VN_CAST(nodep, NodeAssign)) {
+            // Check LHS
+            AstNode* const lhsp = skipConstSels(assignp->lhsp());
+            // LHS is not a VarRef, so might be expensive
+            if (!VN_IS(lhsp, VarRef)) return false;
+
+            // Check RHS
+            AstNode* const rhsp = skipConstSels(assignp->rhsp());
+            // RHS is not a VarRef or Constant so might be expensive
+            if (!VN_IS(rhsp, VarRef) && !VN_IS(rhsp, Const)) return false;
+
+            // Otherwise it is a cheap assignment
+            return true;
+        }
+        // Others are not
+        return false;
     }
 
     // Predicate to check if an expression yields only 0 or 1 (i.e.: a 1-bit value)
@@ -196,23 +577,21 @@ private:
     static AstNode* maskLsb(AstNode* nodep) {
         if (yieldsOneOrZero(nodep)) return nodep;
         // Otherwise apply masking
-        AstNode* const maskp = new AstConst(nodep->fileline(), AstConst::BitTrue());
+        AstNode* const maskp = new AstConst{nodep->fileline(), AstConst::BitTrue()};
         // Mask on left, as conventional
-        return new AstAnd(nodep->fileline(), maskp, nodep);
+        return new AstAnd{nodep->fileline(), maskp, nodep};
     }
 
-    // Fold the RHS expression assuming the given condition state. Unlink bits
+    // Fold the RHS expression of an assignment assuming the given condition state.
-    // from the RHS which is only used once, and can be reused. What remains
+    // Unlink bits from the RHS which is only used once, and can be reused (is an unomdified
-    // of the RHS is expected to be deleted by the caller.
+    // sub-tree). What remains of the RHS is expected to be deleted by the caller.
     AstNode* foldAndUnlink(AstNode* rhsp, bool condTrue) {
         if (rhsp->sameTree(m_mgCondp)) {
-            return new AstConst(rhsp->fileline(), AstConst::BitTrue{}, condTrue);
+            return new AstConst{rhsp->fileline(), AstConst::BitTrue{}, condTrue};
-        } else if (const AstNodeCond* const condp = extractCond(rhsp)) {
+        } else if (const AstNodeCond* const condp = extractCondFromRhs(rhsp)) {
             AstNode* const resp
                 = condTrue ? condp->expr1p()->unlinkFrBack() : condp->expr2p()->unlinkFrBack();
-            if (condp == rhsp) {  //
+            if (condp == rhsp) return resp;
-                return resp;
-            }
             if (const AstAnd* const andp = VN_CAST(rhsp, And)) {
                 UASSERT_OBJ(andp->rhsp() == condp, rhsp, "Should not try to fold this");
                 return new AstAnd{andp->fileline(), andp->lhsp()->cloneTree(false), resp};
@@ -227,17 +606,18 @@ private:
                 return condTrue ? maskLsb(andp->lhsp()->unlinkFrBack())
                                 : new AstConst{rhsp->fileline(), AstConst::BitFalse()};
             }
-        } else if (VN_IS(rhsp, WordSel) || VN_IS(rhsp, VarRef) || VN_IS(rhsp, Const)) {
+        } else if (VN_IS(rhsp, ArraySel) || VN_IS(rhsp, WordSel) || VN_IS(rhsp, VarRef)
+                   || VN_IS(rhsp, Const)) {
             return rhsp->cloneTree(false);
         }
-        rhsp->dumpTree("Don't know how to fold expression: ");
+        // LCOV_EXCL_START
-        rhsp->v3fatalSrc("Don't know how to fold expression");
+        if (debug()) rhsp->dumpTree("Don't know how to fold expression: ");
+        rhsp->v3fatalSrc("Should not try to fold this during conditional merging");
+        // LCOV_EXCL_STOP
     }
 
-    void mergeEnd(int lineno) {
+    void mergeEnd() {
-        UASSERT(m_mgFirstp, "mergeEnd without list " << lineno);
+        UASSERT(m_mgFirstp, "mergeEnd without list");
-        // We might want to recursively merge an AstIf. We stash it in this variable.
-        const AstNodeIf* recursivep = nullptr;
         // Drop leading cheap nodes. These were only added in the hope of finding
         // an earlier reduced form, but we failed to do so.
         while (m_mgFirstp->user2() && m_mgFirstp != m_mgLastp) {
@@ -254,8 +634,11 @@ private:
             m_mgLastp = m_mgLastp->backp();
             --m_listLenght;
             UASSERT_OBJ(m_mgLastp && m_mgLastp->nextp() == nextp, m_mgFirstp,
-                        "Cheap assignment should not be at the front of the list");
+                        "Cheap statement should not be at the front of the list");
         }
+        // If the list contains a single AstNodeIf, we will want to merge its branches.
+        // If so, keep hold of the AstNodeIf in this variable.
+        AstNodeIf* recursivep = nullptr;
         // Merge if list is longer than one node
         if (m_mgFirstp != m_mgLastp) {
             UINFO(6, "MergeCond - First: " << m_mgFirstp << " Last: " << m_mgLastp << endl);
@@ -266,7 +649,7 @@ private:
             // and we also need to keep track of it for comparisons later.
             m_mgCondp = m_mgCondp->cloneTree(false);
             // Create equivalent 'if' statement and insert it before the first node
-            AstIf* const resultp = new AstIf(m_mgCondp->fileline(), m_mgCondp);
+            AstIf* const resultp = new AstIf{m_mgCondp->fileline(), m_mgCondp};
             m_mgFirstp->addHereThisAsNext(resultp);
             // Unzip the list and insert under branches
             AstNode* nextp = m_mgFirstp;
@@ -308,10 +691,12 @@ private:
                     VL_DO_DANGLING(ifp->deleteTree(), ifp);
                 }
             } while (nextp);
-            // Recursively merge the resulting AstIf
+            // Merge the branches of the resulting AstIf after re-analysis
-            recursivep = resultp;
+            if (resultp->ifsp()) m_workQueuep->push(resultp->ifsp());
-        } else if (const AstNodeIf* const ifp = VN_CAST(m_mgFirstp, NodeIf)) {
+            if (resultp->elsesp()) m_workQueuep->push(resultp->elsesp());
-            // There was nothing to merge this AstNodeIf with, but try to merge it's branches
+        } else if (AstNodeIf* const ifp = VN_CAST(m_mgFirstp, NodeIf)) {
+            // There was nothing to merge this AstNodeIf with, so try to merge its branches.
+            // No re-analysis is required for this, so do it directly below
             recursivep = ifp;
         }
         // Reset state
@@ -321,14 +706,13 @@ private:
         m_mgNextp = nullptr;
         AstNode::user1ClearTree();  // Clear marked variables
         AstNode::user2ClearTree();
-        // Merge recursively within the branches
+        // Merge recursively within the branches of an un-merged AstNodeIF
         if (recursivep) {
             iterateAndNextNull(recursivep->ifsp());
-            // Close list, if there is one at the end of the then branch
-            if (m_mgFirstp) mergeEnd(__LINE__);
             iterateAndNextNull(recursivep->elsesp());
-            // Close list, if there is one at the end of the else branch
+            // Close a pending merge to ensure merge state is
-            if (m_mgFirstp) mergeEnd(__LINE__);
+            // reset as expected at the end of this function
+            if (m_mgFirstp) mergeEnd();
         }
     }
 
@@ -351,47 +735,16 @@ private:
         return false;
     }
 
-    // Check if this node is cheap enough that duplicating it in two branches of an
+    bool addToList(AstNodeStmt* nodep, AstNode* condp) {
-    // AstIf and is hence not likely to cause a performance degradation if doing so.
-    bool isCheapNode(AstNode* nodep) const {
-        if (VN_IS(nodep, Comment)) return true;
-        if (const AstNodeAssign* const assignp = VN_CAST(nodep, NodeAssign)) {
-            // Check LHS
-            AstNode* lhsp = assignp->lhsp();
-            while (AstWordSel* const wselp = VN_CAST(lhsp, WordSel)) {
-                // WordSel index is not constant, so might be expensive
-                if (!VN_IS(wselp->bitp(), Const)) return false;
-                lhsp = wselp->fromp();
-            }
-            // LHS is not a VarRef, so might be expensive
-            if (!VN_IS(lhsp, VarRef)) return false;
-
-            // Check RHS
-            AstNode* rhsp = assignp->rhsp();
-            while (AstWordSel* const wselp = VN_CAST(rhsp, WordSel)) {
-                // WordSel index is not constant, so might be expensive
-                if (!VN_IS(wselp->bitp(), Const)) return false;
-                rhsp = wselp->fromp();
-            }
-            // RHS is not a VarRef or Constant so might be expensive
-            if (!VN_IS(rhsp, VarRef) && !VN_IS(rhsp, Const)) return false;
-
-            // Otherwise it is a cheap assignment
-            return true;
-        }
-        return false;
-    }
-
-    bool addToList(AstNode* nodep, AstNode* condp, int line) {
         // Set up head of new list if node is first in list
         if (!m_mgFirstp) {
-            UASSERT_OBJ(condp, nodep, "Cannot start new list without condition " << line);
+            UASSERT_OBJ(condp, nodep, "Cannot start new list without condition");
             // Mark variable references in the condition
             condp->foreach<AstVarRef>([](const AstVarRef* nodep) { nodep->varp()->user1(1); });
             // Now check again if mergeable. We need this to pick up assignments to conditions,
             // e.g.: 'c = c ? a : b' at the beginning of the list, which is in fact not mergeable
             // because it updates the condition. We simply bail on these.
-            if (m_checkMergeable(nodep) != Mergeable::YES) {
+            if ((*m_stmtPropertiesp)(nodep).writesConditionVar()) {
                 // Clear marked variables
                 AstNode::user1ClearTree();
                 // We did not add to the list
@@ -400,11 +753,13 @@ private:
             m_mgFirstp = nodep;
             m_mgCondp = condp;
             m_listLenght = 0;
-            // Add any preceding nodes to the list that would allow us to extend the merge range
+            // Add any preceding nodes to the list that would allow us to extend the merge
-            for (;;) {
+            // range
-                AstNode* const backp = m_mgFirstp->backp();
+            while (true) {
+                AstNodeStmt* const backp = VN_CAST(m_mgFirstp->backp(), NodeStmt);
                 if (!backp || backp->nextp() != m_mgFirstp) break;  // Don't move up the tree
-                if (m_checkMergeable(backp) != Mergeable::YES) break;
+                const StmtProperties& props = (*m_stmtPropertiesp)(backp);
+                if (props.m_isFence || props.writesConditionVar()) break;
                 if (isSimplifiableNode(backp)) {
                     ++m_listLenght;
                     m_mgFirstp = backp;
@@ -424,59 +779,53 @@ private:
         // Set up expected next node in list.
         m_mgNextp = nodep->nextp();
         // If last under parent, done with current list
-        if (!m_mgNextp) mergeEnd(__LINE__);
+        if (!m_mgNextp) mergeEnd();
         // We did add to the list
         return true;
     }
 
     // If this node is the next expected node and is helpful to add to the list, do so,
     // otherwise end the current merge. Return ture if added, false if ended merge.
-    bool addIfHelpfulElseEndMerge(AstNode* nodep) {
+    bool addIfHelpfulElseEndMerge(AstNodeStmt* nodep) {
         UASSERT_OBJ(m_mgFirstp, nodep, "List must be open");
         if (m_mgNextp == nodep) {
             if (isSimplifiableNode(nodep)) {
-                if (addToList(nodep, nullptr, __LINE__)) return true;
+                if (addToList(nodep, nullptr)) return true;
             } else if (isCheapNode(nodep)) {
                 nodep->user2(1);
-                if (addToList(nodep, nullptr, __LINE__)) return true;
+                if (addToList(nodep, nullptr)) return true;
             }
         }
         // Not added to list, so we are done with the current list
-        mergeEnd(__LINE__);
+        mergeEnd();
         return false;
     }
 
-    bool checkOrMakeMergeable(AstNode* nodep) {
+    bool checkOrMakeMergeable(const AstNodeStmt* nodep) {
-        const Mergeable reason = m_checkMergeable(nodep);
+        const StmtProperties& props = (*m_stmtPropertiesp)(nodep);
-        // If meregeable, we are done
+        if (props.m_isFence) return false;  // Fence node never mergeable
-        if (reason == Mergeable::YES) return true;
+        // If the statement writes a condition variable of a pending merge,
-        // Node not mergeable.
+        // we must end the pending merge
-        // If no current list, then this node is just special, move on.
+        if (m_mgFirstp && props.writesConditionVar()) mergeEnd();
-        if (!m_mgFirstp) return false;
+        return true;  // Now surely mergeable
-        // Otherwise finish current list
-        mergeEnd(__LINE__);
-        // If a tree was not mergeable due to an assignment to a condition,
-        // then finishing the current list makes it mergeable again.
-        return reason == Mergeable::NO_COND_ASSIGN;
     }
 
-    void mergeEndIfIncompatible(AstNode* nodep, AstNode* condp) {
+    void mergeEndIfIncompatible(const AstNode* nodep, const AstNode* condp) {
         if (m_mgFirstp && (m_mgNextp != nodep || !condp->sameTree(m_mgCondp))) {
             // Node in different list, or has different condition. Finish current list.
-            mergeEnd(__LINE__);
+            mergeEnd();
         }
     }
 
     // VISITORS
     virtual void visit(AstNodeAssign* nodep) override {
-        AstNode* const rhsp = nodep->rhsp();
+        if (AstNode* const condp = (*m_stmtPropertiesp)(nodep).m_condp) {
-        if (const AstNodeCond* const condp = extractCond(rhsp)) {
             // Check if mergeable
             if (!checkOrMakeMergeable(nodep)) return;
             // Close potentially incompatible pending merge
-            mergeEndIfIncompatible(nodep, condp->condp());
+            mergeEndIfIncompatible(nodep, condp);
             // Add current node
-            addToList(nodep, condp->condp(), __LINE__);
+            addToList(nodep, condp);
         } else if (m_mgFirstp) {
             addIfHelpfulElseEndMerge(nodep);
         }
@@ -493,21 +842,22 @@ private:
         // Close potentially incompatible pending merge
         mergeEndIfIncompatible(nodep, nodep->condp());
         // Add current node
-        addToList(nodep, nodep->condp(), __LINE__);
+        addToList(nodep, nodep->condp());
+    }
+
+    virtual void visit(AstNodeStmt* nodep) override {
+        if (m_mgFirstp && addIfHelpfulElseEndMerge(nodep)) return;
+        iterateChildren(nodep);
+    }
+
+    virtual void visit(AstCFunc* nodep) override {
+        // Merge function body
+        if (nodep->stmtsp()) process(nodep->stmtsp());
     }
 
     // For speed, only iterate what is necessary.
     virtual void visit(AstNetlist* nodep) override { iterateAndNextNull(nodep->modulesp()); }
     virtual void visit(AstNodeModule* nodep) override { iterateAndNextNull(nodep->stmtsp()); }
-    virtual void visit(AstCFunc* nodep) override {
-        iterateChildren(nodep);
-        // Close list, if there is one at the end of the function
-        if (m_mgFirstp) mergeEnd(__LINE__);
-    }
-    virtual void visit(AstNodeStmt* nodep) override {
-        if (m_mgFirstp && addIfHelpfulElseEndMerge(nodep)) return;
-        iterateChildren(nodep);
-    }
     virtual void visit(AstNode* nodep) override {}
 
 public:
@@ -520,6 +870,8 @@ public:
     }
 };
 
+}  // namespace
+
 //######################################################################
 // MergeConditionals class functions
 

test_regress/t/t_merge_cond.pl

@@ -21,11 +21,11 @@ execute(
 if ($Self->{vlt}) {
     # Note, with vltmt this might be split differently, so only checking vlt
     file_grep($Self->{stats}, qr/Optimizations, MergeCond merges\s+(\d+)/i,
-              10);
+              9);
     file_grep($Self->{stats}, qr/Optimizations, MergeCond merged items\s+(\d+)/i,
               580);
     file_grep($Self->{stats}, qr/Optimizations, MergeCond longest merge\s+(\d+)/i,
-              64);
+              128);
 }
 
 ok(1);

test_regress/t/t_merge_cond_blowup.pl

@@ -0,0 +1,34 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2022 by Geza Lore. 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
+
+scenarios(vlt => 1);
+
+# TODO: This takes excessively long on vltmt, this should be fixed
+
+compile(
+    verilator_flags2 => ["--unroll-count 1000000000", "--output-split 0", "--stats"],
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+if ($Self->{vlt}) {
+    # Note, with vltmt this might be split differently, so only checking vlt
+    file_grep($Self->{stats}, qr/Optimizations, MergeCond merges\s+(\d+)/i,
+              500);   # V3MergeCond.cpp MAX_DISTANCE
+    file_grep($Self->{stats}, qr/Optimizations, MergeCond merged items\s+(\d+)/i,
+              1000);  # V3MergeCond.cpp MAX_DISTANCE *2
+    file_grep($Self->{stats}, qr/Optimizations, MergeCond longest merge\s+(\d+)/i,
+              2);
+}
+
+ok(1);
+1;

test_regress/t/t_merge_cond_blowup.v

@@ -0,0 +1,55 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2022 by Geza Lore.
+// SPDX-License-Identifier: CC0-1.0
+
+module t (/*AUTOARG*/
+   // Inputs
+   clk
+   );
+   input clk;
+
+   localparam int N = 4096;
+
+   integer cyc = 0;
+   reg [63:0] crc= 64'h5aef0c8d_d70a4497;
+
+   always @ (posedge clk) begin
+      cyc <= cyc + 1;
+      crc <= {crc[62:0], crc[63] ^ crc[2] ^ crc[0]};
+
+      if (cyc==99) begin
+         $write("*-* All Finished *-*\n");
+         $finish;
+      end
+   end
+
+   reg a [N-1:0];
+   reg b [N-1:0];
+
+   // This yields pathological complexity for the current conditional merging
+   // algorithm. Note in practice, other parts of the compiler blow up on this
+   // code far earlier than the conditional merging, but here we go anyway.
+   generate
+      genvar i;
+      for (i = 0 ; i < N ; i = i + 1) begin
+        always @(posedge clk) a[i] <= (crc + 64'(i)) == 0 ? crc[(i+16)%64] : crc[(i+32)%64];
+      end
+      for (i = 0 ; i < N ; i = i + 1) begin
+        always @(posedge clk) b[i] <= (crc + 64'(i)) == 0 ? crc[(i+16)%64] : crc[(i+32)%64];
+      end
+   endgenerate
+
+   always @(posedge clk) begin
+      if (cyc >= 2) begin
+        for (int i = 0 ; i < N ; i = i + 1) begin
+          if (a[i] !== b[i]) begin
+            $write("%%Error: %s:%0d: cyc=%0d i=%0d a[i]='h%x b[i]='h%x\n", `__FILE__,`__LINE__, cyc, i, a[i], b[i]);
+            $stop;
+          end
+        end
+      end
+   end
+
+endmodule