diff --git a/src/V3MergeCond.cpp b/src/V3MergeCond.cpp index 62ef02196..57b5dc511 100644 --- a/src/V3MergeCond.cpp +++ b/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 - = false; // State tracking whether tree being processed is a mergeable condition + 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() - 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) { - m_mergeable = true; - iterateChildrenConst(const_cast(node)); - return m_mergeable; + Mergeable operator()(const AstNode* node) { + m_condAssign = false; + m_impure = false; + iterateChildrenConst(const_cast(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, - // 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. Ideally we would only do this iff the node is known not to be 1-bit - // wide, but working that out here is a bit difficult. As this masking is - // rarely required (only when trying to merge a "cond & value" with an - // earlier ternary), we will just always mask it for safety. + // Predicate to check if an expression yields only 0 or 1 (i.e.: a 1-bit value) + static bool yieldsOneOrZero(const AstNode* nodep) { + UASSERT_OBJ(!nodep->isWide(), nodep, "Cannot handle wide nodes"); + if (const AstConst* const constp = VN_CAST_CONST(nodep, Const)) { + return constp->num().toUQuad() <= 1; + } + 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() { - UASSERT(m_mgFirstp, "mergeEnd without list"); + void mergeEnd(int lineno) { + 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 - = new AstIf(m_mgCondp->fileline(), m_mgCondp->unlinkFrBack(), nullptr, nullptr); - m_mgFirstp->replaceWith(ifp); - ifp->addNextHere(m_mgFirstp); + AstIf* const resultp = new AstIf(m_mgCondp->fileline(), m_mgCondp, nullptr, nullptr); + m_mgFirstp->addHereThisAsNext(resultp); // 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) - AstNodeAssign* const thenp = VN_CAST(currp, NodeAssign); - AstNode* const rhsp = thenp->rhsp()->unlinkFrBack(); - AstNodeAssign* const elsep = thenp->cloneTree(false); - // Construct the new RHSs and add to branches - thenp->rhsp(foldAndUnlink(rhsp, true)); - elsep->rhsp(foldAndUnlink(rhsp, false)); - ifp->addIfsp(thenp); - ifp->addElsesp(elsep); - // Cleanup - VL_DO_DANGLING(rhsp->deleteTree(), rhsp); + if (AstNodeAssign* const assignp = VN_CAST(currp, NodeAssign)) { + // Unlink RHS and clone to get the 2 assignments (reusing assignp) + AstNode* const rhsp = assignp->rhsp()->unlinkFrBack(); + AstNodeAssign* const thenp = assignp; + AstNodeAssign* const elsep = assignp->cloneTree(false); + // Construct the new RHSs and add to branches + thenp->rhsp(foldAndUnlink(rhsp, true)); + elsep->rhsp(foldAndUnlink(rhsp, false)); + resultp->addIfsp(thenp); + resultp->addElsesp(elsep); + // 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 - // by V3Order before always blocks) + // Set up expected next node in list. 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)) { - // Node not mergeable. - // If no current list, then this node is just special, move on. - if (!m_mgFirstp) return; - // 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(); - } + // Check if mergeable + if (!checkOrMakeMergeable(nodep)) return; + // Close potentially incompatible pending merge + mergeEndIfIncompatible(nodep, condp->condp()); // 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. - // 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(); + addIfHelpfulElseEndMerge(nodep); } } - 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: diff --git a/test_regress/t/t_optm_if_cond.pl b/test_regress/t/t_optm_if_cond.pl index 3c5833594..b67f09305 100755 --- a/test_regress/t/t_optm_if_cond.pl +++ b/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}) {