Improve V3MergeCond

- Merge AstNodeIf nodes as well (not just assignment from AstCond)
- Merge merged results recursively (optimizes nested conditionals/ifs)
- Only checking mergeability once per node.
- Don't add redundant masking
- Duplicate cheap statements in both branches, if doing so yields a
  larger merge
- Include reduced nodes before the starting conditional in the merge

src/V3MergeCond.cpp

@@ -40,6 +40,8 @@
 //      'lhs = cond & value' is actually 'lhs = cond ? value : 1'd0'
 //      'lhs = cond' is actually 'lhs = cond ? 1'd1 : 1'd0'.
 //
+//  Also merges consecutive AstNodeIf statements with the same condition.
+//
 //*************************************************************************
 
 #include "config_build.h"
@@ -52,36 +54,38 @@
 
 //######################################################################
 
+enum class Mergeable {
+    YES,  // Tree can be merged
+    NO_COND_ASSIGN,  // Tree cannot be merged because it contains an assignment to a condition
+    NO_IMPURE  // Tree cannot be merged because it contains an impure node
+};
+
 class CheckMergeableVisitor final : public AstNVisitor {
 private:
     // STATE
-    bool m_mergeable
+    bool m_condAssign = false;  // Does this tree contain an assignment to a condition variable??
-        = false;  // State tracking whether tree being processed is a mergeable condition
+    bool m_impure = false;  // Does this tree contain an impure node?
 
     // METHODS
     VL_DEBUG_FUNC;  // Declare debug()
 
-    void clearMergeable(const AstNode* nodep, const char* reason) {
-        UASSERT_OBJ(m_mergeable, nodep, "Should have short-circuited traversal");
-        m_mergeable = false;
-        UINFO(9, "Clearing mergeable on " << nodep << " due to " << reason << endl);
-    }
-
     // VISITORS
     virtual void visit(AstNode* nodep) override {
-        if (!m_mergeable) return;
+        if (m_impure) return;
         // Clear if node is impure
         if (!nodep->isPure()) {
-            clearMergeable(nodep, "impure");
+            UINFO(9, "Not mergeable due to impure node" << nodep << endl);
+            m_impure = true;
             return;
         }
         iterateChildrenConst(nodep);
     }
     virtual void visit(AstVarRef* nodep) override {
-        if (!m_mergeable) return;
+        if (m_impure || m_condAssign) return;
         // Clear if it's an LValue referencing a marked variable
         if (nodep->access().isWriteOrRW() && nodep->varp()->user1()) {
-            clearMergeable(nodep, "might modify condition");
+            UINFO(9, "Not mergeable due assignment to condition" << nodep << endl);
+            m_condAssign = true;
         }
     }
 
@@ -91,10 +95,17 @@ public:
     // Return false if this node should not be merged at all because:
     // - It contains an impure expression
     // - It contains an LValue referencing the condition
-    bool operator()(const AstNodeAssign* node) {
+    Mergeable operator()(const AstNode* node) {
-        m_mergeable = true;
+        m_condAssign = false;
-        iterateChildrenConst(const_cast<AstNodeAssign*>(node));
+        m_impure = false;
-        return m_mergeable;
+        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;
+        }
     }
 };
 
@@ -118,8 +129,10 @@ public:
 class MergeCondVisitor final : public AstNVisitor {
 private:
     // NODE STATE
-    // AstVar::user1 -> Flag set for variables referenced by m_mgCondp
+    // AstVar::user1        -> Flag set for variables referenced by m_mgCondp
+    // AstNode::user2       -> Flag marking node as included in merge because cheap to duplicate
     AstUser1InUse m_user1InUse;
+    AstUser2InUse m_user2InUse;
 
     // STATE
     VDouble0 m_statMerges;  // Statistic tracking
@@ -153,16 +166,57 @@ private:
         return nullptr;
     }
 
-    // Apply (_ & 1'b1). This is necessary because this pass is after V3Clean,
+    // Predicate to check if an expression yields only 0 or 1 (i.e.: a 1-bit value)
-    // and sometimes we have an AstAnd with a 1-bit condition on one side, but
+    static bool yieldsOneOrZero(const AstNode* nodep) {
-    // a more than 1-bit value on the other side, so we need to keep only the
+        UASSERT_OBJ(!nodep->isWide(), nodep, "Cannot handle wide nodes");
-    // LSB. Ideally we would only do this iff the node is known not to be 1-bit
+        if (const AstConst* const constp = VN_CAST_CONST(nodep, Const)) {
-    // wide, but working that out here is a bit difficult. As this masking is
+            return constp->num().toUQuad() <= 1;
-    // rarely required (only when trying to merge a "cond & value" with an
+        }
-    // earlier ternary), we will just always mask it for safety.
+        if (const AstVarRef* const vrefp = VN_CAST_CONST(nodep, VarRef)) {
+            AstVar* const varp = vrefp->varp();
+            return varp->widthMin() == 1 && !varp->dtypep()->isSigned();
+        }
+        if (const AstShiftR* const shiftp = VN_CAST_CONST(nodep, ShiftR)) {
+            // Shift right by width - 1 or more
+            if (const AstConst* const constp = VN_CAST_CONST(shiftp->rhsp(), Const)) {
+                const AstVarRef* const vrefp = VN_CAST_CONST(shiftp->lhsp(), VarRef);
+                const int width = vrefp && !vrefp->varp()->dtypep()->isSigned()
+                                      ? vrefp->varp()->widthMin()
+                                      : shiftp->width();
+                if (constp->toSInt() >= width - 1) return true;
+            }
+            return false;
+        }
+        if (VN_IS(nodep, Eq) || VN_IS(nodep, Neq) || VN_IS(nodep, Lt) || VN_IS(nodep, Lte)
+            || VN_IS(nodep, Gt) || VN_IS(nodep, Gte)) {
+            return true;
+        }
+        if (const AstNodeBiop* const biopp = VN_CAST_CONST(nodep, NodeBiop)) {
+            if (VN_IS(nodep, And))
+                return yieldsOneOrZero(biopp->lhsp()) || yieldsOneOrZero(biopp->rhsp());
+            if (VN_IS(nodep, Or) || VN_IS(nodep, Xor))
+                return yieldsOneOrZero(biopp->lhsp()) && yieldsOneOrZero(biopp->rhsp());
+            return false;
+        }
+        if (const AstNodeCond* const condp = VN_CAST_CONST(nodep, NodeCond)) {
+            return yieldsOneOrZero(condp->expr1p()) && yieldsOneOrZero(condp->expr2p());
+        }
+        if (const AstCCast* const castp = VN_CAST_CONST(nodep, CCast)) {
+            // Cast never sign extends
+            return yieldsOneOrZero(castp->lhsp());
+        }
+        return false;
+    }
+
+    // Apply (1'b1 & _) cleaning mask if necessary. This is required because this pass is after
+    // V3Clean, and sometimes we have an AstAnd with a 1-bit condition on one side, but a more
+    // than 1-bit value on the other side, so we need to keep only the LSB.
     static AstNode* maskLsb(AstNode* nodep) {
+        if (yieldsOneOrZero(nodep)) return nodep;
+        // Otherwise apply masking
         AstNode* const maskp = new AstConst(nodep->fileline(), AstConst::BitTrue());
-        return new AstAnd(nodep->fileline(), nodep, maskp);
+        // Mask on left, as conventional
+        return new AstAnd(nodep->fileline(), maskp, nodep);
     }
 
     // Fold the RHS expression assuming the given condition state. Unlink bits
@@ -191,23 +245,47 @@ 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)) {
+            return rhsp->cloneTree(false);
         }
+        rhsp->dumpTree("Don't know how to fold expression: ");
         rhsp->v3fatalSrc("Don't know how to fold expression");
     }
 
-    void mergeEnd() {
+    void mergeEnd(int lineno) {
-        UASSERT(m_mgFirstp, "mergeEnd without list");
+        UASSERT(m_mgFirstp, "mergeEnd without list " << lineno);
+        // We might want to recursively merge an AstIf. We stash it in this variable.
+        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) {
+            AstNode* const backp = m_mgFirstp;
+            m_mgFirstp = m_mgFirstp->nextp();
+            --m_listLenght;
+            UASSERT_OBJ(m_mgFirstp && m_mgFirstp->backp() == backp, m_mgLastp,
+                        "The list should have a non-cheap element");
+        }
+        // Drop trailing cheap nodes. These were only added in the hope of finding
+        // a later conditional to merge, but we failed to do so.
+        while (m_mgLastp->user2() && m_mgFirstp != m_mgLastp) {
+            AstNode* const nextp = m_mgLastp;
+            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");
+        }
         // Merge if list is longer than one node
         if (m_mgFirstp != m_mgLastp) {
             UINFO(6, "MergeCond - First: " << m_mgFirstp << " Last: " << m_mgLastp << endl);
             ++m_statMerges;
             if (m_listLenght > m_statLongestList) m_statLongestList = m_listLenght;
 
+            // We need a copy of the condition in the new equivalent 'if' statement,
+            // 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 ifp
+            AstIf* const resultp = new AstIf(m_mgCondp->fileline(), m_mgCondp, nullptr, nullptr);
-                = new AstIf(m_mgCondp->fileline(), m_mgCondp->unlinkFrBack(), nullptr, nullptr);
+            m_mgFirstp->addHereThisAsNext(resultp);
-            m_mgFirstp->replaceWith(ifp);
-            ifp->addNextHere(m_mgFirstp);
             // Unzip the list and insert under branches
             AstNode* nextp = m_mgFirstp;
             do {
@@ -222,18 +300,37 @@ private:
                 }
                 // Count
                 ++m_statMergedItems;
-                // Unlink RHS and clone to get the 2 assignments (reusing currp)
+                if (AstNodeAssign* const assignp = VN_CAST(currp, NodeAssign)) {
-                AstNodeAssign* const thenp = VN_CAST(currp, NodeAssign);
+                    // Unlink RHS and clone to get the 2 assignments (reusing assignp)
-                AstNode* const rhsp = thenp->rhsp()->unlinkFrBack();
+                    AstNode* const rhsp = assignp->rhsp()->unlinkFrBack();
-                AstNodeAssign* const elsep = thenp->cloneTree(false);
+                    AstNodeAssign* const thenp = assignp;
-                // Construct the new RHSs and add to branches
+                    AstNodeAssign* const elsep = assignp->cloneTree(false);
-                thenp->rhsp(foldAndUnlink(rhsp, true));
+                    // Construct the new RHSs and add to branches
-                elsep->rhsp(foldAndUnlink(rhsp, false));
+                    thenp->rhsp(foldAndUnlink(rhsp, true));
-                ifp->addIfsp(thenp);
+                    elsep->rhsp(foldAndUnlink(rhsp, false));
-                ifp->addElsesp(elsep);
+                    resultp->addIfsp(thenp);
-                // Cleanup
+                    resultp->addElsesp(elsep);
-                VL_DO_DANGLING(rhsp->deleteTree(), rhsp);
+                    // Cleanup
+                    VL_DO_DANGLING(rhsp->deleteTree(), rhsp);
+                } else {
+                    AstNodeIf* const ifp = VN_CAST(currp, NodeIf);
+                    UASSERT_OBJ(ifp, currp, "Must be AstNodeIf");
+                    // Move branch contents under new if
+                    if (AstNode* const listp = ifp->ifsp()) {
+                        resultp->addIfsp(listp->unlinkFrBackWithNext());
+                    }
+                    if (AstNode* const listp = ifp->elsesp()) {
+                        resultp->addElsesp(listp->unlinkFrBackWithNext());
+                    }
+                    // Cleanup
+                    VL_DO_DANGLING(ifp->deleteTree(), ifp);
+                }
             } while (nextp);
+            // Recursively merge the resulting AstIf
+            recursivep = resultp;
+        } else if (AstNodeIf* const ifp = VN_CAST(m_mgFirstp, NodeIf)) {
+            // There was nothing to merge this AstNodeIf with, but try to merge it's branches
+            recursivep = ifp;
         }
         // Reset state
         m_mgFirstp = nullptr;
@@ -241,82 +338,185 @@ private:
         m_mgLastp = nullptr;
         m_mgNextp = nullptr;
         m_markVars.clear();
+        AstNode::user2ClearTree();
+        // Merge recursively within the branches
+        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
+            if (m_mgFirstp) mergeEnd(__LINE__);
+        }
     }
 
-    void addToList(AstNode* nodep, AstNode* condp) {
+    // Check if the node can be simplified if included under the if
+    bool isSimplifiableNode(AstNode* nodep) {
+        UASSERT_OBJ(m_mgFirstp, nodep, "Cannot check with empty list");
+        if (AstNodeAssign* const assignp = VN_CAST(nodep, NodeAssign)) {
+            // If it's an assignment to a 1-bit signal, try reduced forms
+            if (assignp->lhsp()->widthMin() == 1) {
+                // Is it a 'lhs = cond & value' or 'lhs = value & cond'?
+                if (AstAnd* const andp = VN_CAST(assignp->rhsp(), And)) {
+                    if (andp->lhsp()->sameTree(m_mgCondp) || andp->rhsp()->sameTree(m_mgCondp)) {
+                        return true;
+                    }
+                }
+                // Is it simply 'lhs = cond'?
+                if (assignp->rhsp()->sameTree(m_mgCondp)) return true;
+            }
+        }
+        return false;
+    }
+
+    // Check if this node is cheap enough that duplicating it in two branches of an
+    // 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 (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;
+    }
+
+    void 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");
+            UASSERT_OBJ(condp, nodep, "Cannot start new list without condition " << line);
             m_mgFirstp = nodep;
             m_mgCondp = condp;
             m_listLenght = 0;
             m_markVars.mark(condp);
+            // Add any preceding nodes to the list that would allow us to extend the merge range
+            for (;;) {
+                AstNode* const backp = m_mgFirstp->backp();
+                if (!backp || backp->nextp() != m_mgFirstp) break;  // Don't move up the tree
+                if (m_checkMergeable(backp) != Mergeable::YES) break;
+                if (isSimplifiableNode(backp)) {
+                    ++m_listLenght;
+                    m_mgFirstp = backp;
+                } else if (isCheapNode(backp)) {
+                    backp->user2(1);
+                    ++m_listLenght;
+                    m_mgFirstp = backp;
+                } else {
+                    break;
+                }
+            }
         }
         // Add node
         ++m_listLenght;
         // Track end of list
         m_mgLastp = nodep;
-        // Set up expected next node in list. Skip over any comments, (inserted
+        // Set up expected next node in list.
-        // by V3Order before always blocks)
         m_mgNextp = nodep->nextp();
-        while (m_mgNextp && VN_IS(m_mgNextp, Comment)) { m_mgNextp = m_mgNextp->nextp(); }
         // If last under parent, done with current list
-        if (!m_mgNextp) mergeEnd();
+        if (!m_mgNextp) mergeEnd(__LINE__);
+    }
+
+    // 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) {
+        UASSERT_OBJ(m_mgFirstp, nodep, "List must be open");
+        if (m_mgNextp == nodep) {
+            if (isSimplifiableNode(nodep)) {
+                addToList(nodep, nullptr, __LINE__);
+                return true;
+            }
+            if (isCheapNode(nodep)) {
+                nodep->user2(1);
+                addToList(nodep, nullptr, __LINE__);
+                return true;
+            }
+        }
+        // Not added to list, so we are done with the current list
+        mergeEnd(__LINE__);
+        return false;
+    }
+
+    bool checkOrMakeMergeable(AstNode* nodep) {
+        const Mergeable reason = m_checkMergeable(nodep);
+        // If meregeable, we are done
+        if (reason == Mergeable::YES) return true;
+        // Node not mergeable.
+        // If no current list, then this node is just special, move on.
+        if (!m_mgFirstp) return false;
+        // 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) {
+        if (m_mgFirstp && (m_mgNextp != nodep || !condp->sameTree(m_mgCondp))) {
+            // Node in different list, or has different condition. Finish current list.
+            mergeEnd(__LINE__);
+        }
     }
 
     // VISITORS
     virtual void visit(AstNodeAssign* nodep) override {
         AstNode* const rhsp = nodep->rhsp();
         if (AstNodeCond* const condp = extractCond(rhsp)) {
-            if (!m_checkMergeable(nodep)) {
+            // Check if mergeable
-                // Node not mergeable.
+            if (!checkOrMakeMergeable(nodep)) return;
-                // If no current list, then this node is just special, move on.
+            // Close potentially incompatible pending merge
-                if (!m_mgFirstp) return;
+            mergeEndIfIncompatible(nodep, condp->condp());
-                // Otherwise finish current list
-                mergeEnd();
-                // Finishing the list might make the node mergeable again, e.g.
-                // if the reason we could not merge was due to the condition
-                // being assigned, so check again and stop only if still no.
-                if (!m_checkMergeable(nodep)) return;
-            }
-            if (m_mgFirstp && (m_mgNextp != nodep || !condp->condp()->sameTree(m_mgCondp))) {
-                // Node in different list, or has different condition.
-                // Finish current list, addToList will start a new one.
-                mergeEnd();
-            }
             // Add current node
-            addToList(nodep, condp->condp());
+            addToList(nodep, condp->condp(), __LINE__);
         } else if (m_mgFirstp) {
-            // RHS is not a conditional, but we already started a list.
+            addIfHelpfulElseEndMerge(nodep);
-            // If it's a 1-bit signal, and a mergeable assignment, try reduced forms
-            if (m_mgNextp == nodep && rhsp->widthMin() == 1 && m_checkMergeable(nodep)) {
-                // Is it a 'lhs = cond & value' or 'lhs = value & cond'?
-                if (AstAnd* const andp = VN_CAST(rhsp, And)) {
-                    if (andp->lhsp()->sameTree(m_mgCondp) || andp->rhsp()->sameTree(m_mgCondp)) {
-                        addToList(nodep, nullptr);
-                        return;
-                    }
-                }
-                // Is it simply 'lhs = cond'?
-                if (rhsp->sameTree(m_mgCondp)) {
-                    addToList(nodep, nullptr);
-                    return;
-                }
-            }
-            // Not added to list, so we are done with the current list
-            mergeEnd();
         }
     }
-    virtual void visit(AstComment*) override {}  // Skip over comments
+
+    virtual void visit(AstNodeIf* nodep) override {
+        // Check if mergeable
+        if (!checkOrMakeMergeable(nodep)) {
+            // If not mergeable, try to merge the branches
+            iterateAndNextNull(nodep->ifsp());
+            iterateAndNextNull(nodep->elsesp());
+            return;
+        }
+        // Close potentially incompatible pending merge
+        mergeEndIfIncompatible(nodep, nodep->condp());
+        // Add current node
+        addToList(nodep, nodep->condp(), __LINE__);
+    }
+
     // 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();
+        if (m_mgFirstp) mergeEnd(__LINE__);
+    }
+    virtual void visit(AstNodeStmt* nodep) override {
+        if (m_mgFirstp && addIfHelpfulElseEndMerge(nodep)) return;
+        iterateChildren(nodep);
     }
-    virtual void visit(AstNodeStmt* nodep) override { iterateChildren(nodep); }
     virtual void visit(AstNode* nodep) override {}
 
 public:

test_regress/t/t_optm_if_cond.pl

@@ -11,7 +11,7 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di
 scenarios(vlt => 1);
 
 compile(
-    verilator_flags2 => ['--stats'],
+    verilator_flags2 => ['--stats', "-Ow"],
     );
 
 if ($Self->{vlt_all}) {