From c079aa0b17679eac94668f2365ef6e07c660bdef Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Mon, 1 Jun 2026 20:25:41 +0100 Subject: [PATCH] Optimize wide conditional expansion in V3Premit (#7691) V3Premit extracts wide sub-expressions via temporaries, which is needed for emitting wide operations to C++ (calls to `VL_*_W`). The previous version used to extract both branches of an AstCond unconditionally, meaning both branches were fully evaluated. Rewriting the AstCond into an AstIf instead enables evaluating only the required branch. While this does limit V3Subst, overall the resulting code is ~3% faster, and contains ~25% fewer branches on a large design. --- src/V3Const.cpp | 3 + src/V3Premit.cpp | 113 ++++++++++++++++++++------ test_regress/t/t_flag_expand_limit.py | 2 +- 3 files changed, 93 insertions(+), 25 deletions(-) diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 949ef9f79..43df3fac9 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -1653,6 +1653,9 @@ class ConstVisitor final : public VNVisitor { if (VN_IS(thensp->rhsp()->dtypep()->skipRefp(), UnpackArrayDType)) return false; if (VN_IS(elsesp->rhsp()->dtypep()->skipRefp(), UnpackArrayDType)) return false; if (m_underRecFunc) return false; // This optimization may lead to infinite recursion + if (m_doCpp) { // Do not combine ifs cretaed by V3Premit + if (VN_IS(thensp->lhsp(), VarRef) && thensp->lhsp()->isWide()) return false; + } // Only do it if not calls and both pure, otherwise undoes V3LiftExpr return !VN_IS(thensp->rhsp(), NodeFTaskRef) // && !VN_IS(elsesp->rhsp(), NodeFTaskRef) // diff --git a/src/V3Premit.cpp b/src/V3Premit.cpp index 199e6f5b7..50c23da38 100644 --- a/src/V3Premit.cpp +++ b/src/V3Premit.cpp @@ -55,14 +55,48 @@ class PremitVisitor final : public VNVisitor { bool m_assignLhs = false; // Inside assignment lhs, don't breakup extracts // METHODS - void checkNode(AstNodeExpr* nodep) { + bool needsTemp(AstNodeExpr* nodep) { // Consider adding a temp for this expression. - if (!m_stmtp) return; // Not under a statement - if (nodep->user1SetOnce()) return; // Already processed - if (!nodep->isWide()) return; // Not wide - if (m_assignLhs) return; // This is an lvalue! + if (!m_stmtp) return false; // Not under a statement + if (nodep->user1SetOnce()) return false; // Already processed + if (!nodep->isWide()) return false; // Not wide + if (m_assignLhs) return false; // This is an lvalue! UASSERT_OBJ(!VN_IS(nodep->firstAbovep(), ArraySel), nodep, "Should have been ignored"); - createTemp(nodep); + return true; + } + + void checkNode(AstNodeExpr* nodep) { + if (needsTemp(nodep)) createTemp(nodep); + } + + AstVarRef* isRhsOfAssignToVar(AstNodeExpr* nodep) { + // If enclosing statement is an assign + AstNodeAssign* const assignp = VN_CAST(m_stmtp, NodeAssign); + if (!assignp) return nullptr; + // of which 'nodep' is the RHS of + if (assignp->rhsp() != nodep) return nullptr; + // and the LHS is a sipmple variable reference + AstVarRef* const lRefp = VN_CAST(assignp->lhsp(), VarRef); + if (!lRefp) return nullptr; + AstVar* const varp = lRefp->varp(); + // And the RHS does not use the same variable + if (nodep->exists([varp](const AstVarRef* refp) { return refp->varp() == varp; })) { + return nullptr; + } + // Then return the LHS reference + return lRefp; + } + + // Create a new temporary that can hold the value of the given expression + AstVar* newTmpFor(AstNodeExpr* nodep) { + FileLine* const flp = nodep->fileline(); + const std::string name = "__Vtemp_" + std::to_string(++m_tmpVarCnt); + AstVar* const varp = new AstVar{flp, VVarType::STMTTEMP, name, nodep->dtypep()}; + varp->funcLocal(true); + varp->noReset(true); + m_cfuncp->addVarsp(varp); + ++m_temporaryVarsCreated; + return varp; } AstVar* createTemp(AstNodeExpr* nodep) { @@ -89,16 +123,10 @@ class PremitVisitor final : public VNVisitor { ++m_extractedToConstPool; } else { // Keep as local temporary. - const std::string name = "__Vtemp_" + std::to_string(++m_tmpVarCnt); - varp = new AstVar{flp, VVarType::STMTTEMP, name, nodep->dtypep()}; - varp->funcLocal(true); - varp->noReset(true); - m_cfuncp->addVarsp(varp); - ++m_temporaryVarsCreated; - + varp = newTmpFor(nodep); // Assignment to put before the referencing statement - AstAssign* const assignp - = new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, nodep}; + AstVarRef* const refp = new AstVarRef{flp, varp, VAccess::WRITE}; + AstAssign* const assignp = new AstAssign{flp, refp, nodep}; // Insert before the statement m_stmtp->addHereThisAsNext(assignp); } @@ -189,9 +217,11 @@ class PremitVisitor final : public VNVisitor { // Direct assignment to a simple variable if (VN_IS(nodep->lhsp(), VarRef) && !AstVar::scVarRecurse(nodep->lhsp())) { AstNode* const rhsp = nodep->rhsp(); - // Rhs is already a var ref, so nothing to so + // Rhs is already a var ref, so nothing to do if (VN_IS(rhsp, VarRef) && !AstVar::scVarRecurse(rhsp)) return; - if (!VN_IS(rhsp, Const)) { + if (VN_IS(rhsp, Cond)) { + // Do replace Cond on RHS, even if a simple assignment + } else if (!VN_IS(rhsp, Const)) { // Don't replace the rhs, it's already a simple assignment rhsp->user1(true); } else if (rhsp->width() < STATIC_CONST_MIN_WIDTH) { @@ -210,6 +240,15 @@ class PremitVisitor final : public VNVisitor { m_assignLhs = true; // Restored by VL_RESTORER in START_STATEMENT_OR_RETURN iterateAndNextNull(nodep->lhsp()); + + // It's possible we end up with an 'a' = 'a' after expanding an AstCond + if (AstVarRef* const rhsp = VN_CAST(nodep->rhsp(), VarRef)) { + if (AstVarRef* const lhsp = VN_CAST(nodep->lhsp(), VarRef)) { + if (lhsp->varp() == rhsp->varp()) { + VL_DO_DANGLING(pushDeletep(nodep->unlinkFrBack()), nodep); + } + } + } } void visit(AstDisplay* nodep) override { START_STATEMENT_OR_RETURN(nodep); @@ -309,15 +348,41 @@ class PremitVisitor final : public VNVisitor { checkNode(nodep); } void visit(AstCond* nodep) override { - iterateChildren(nodep); - if (nodep->thenp()->isWide() && !VN_IS(nodep->condp(), Const) - && !VN_IS(nodep->condp(), VarRef)) { - // We're going to need the expression several times in the expanded code, - // so might as well make it a common expression - createTemp(nodep->condp()); + // Convert AstCond to AstIf in order to avoid evaluating + // sub-expressions in both branches unconditionally. + if (needsTemp(nodep)) { + // Check if LHS variable could be used directly + AstVarRef* const lRefp = isRhsOfAssignToVar(nodep); + // If not, create a new temporary variable + AstVar* varp = lRefp ? lRefp->varp() : newTmpFor(nodep); + // Can't substitute across basic blocks + varp->noSubst(true); + + FileLine* const flp = nodep->fileline(); + // Create 'then' assignment + AstVarRef* const thenRefp = new AstVarRef{flp, varp, VAccess::WRITE}; + if (lRefp) thenRefp->selfPointer(lRefp->selfPointer()); + AstNodeExpr* const thenExprp = nodep->thenp()->unlinkFrBack(); + AstAssign* const thenAsspp = new AstAssign{flp, thenRefp, thenExprp}; + // Create 'else' assignment + AstVarRef* const elseRefp = new AstVarRef{flp, varp, VAccess::WRITE}; + if (lRefp) elseRefp->selfPointer(lRefp->selfPointer()); + AstNodeExpr* const elseExprp = nodep->elsep()->unlinkFrBack(); + AstAssign* const elseAsspp = new AstAssign{flp, elseRefp, elseExprp}; + // Creae 'if' and insert it before the statement + AstNodeExpr* const condp = nodep->condp()->unlinkFrBack(); + AstIf* const ifp = new AstIf{flp, condp, thenAsspp, elseAsspp}; + m_stmtp->addHereThisAsNext(ifp); + // Replace the AstCond with the result variable + nodep->replaceWith(new AstVarRef{flp, varp, VAccess::READ}); + VL_DO_DANGLING(pushDeletep(nodep), nodep); + // Splitting to multiple statements can change purity VIsCached::clearCacheTree(); + // Iterate the resulting assignments + iterate(ifp); + return; } - checkNode(nodep); + iterateChildren(nodep); } void visit(AstSFormatF* nodep) override { iterateChildren(nodep); diff --git a/test_regress/t/t_flag_expand_limit.py b/test_regress/t/t_flag_expand_limit.py index 8cacdb1ca..ec0cc54a6 100755 --- a/test_regress/t/t_flag_expand_limit.py +++ b/test_regress/t/t_flag_expand_limit.py @@ -13,6 +13,6 @@ test.scenarios('vlt') test.compile(verilator_flags2=['--expand-limit 1 --stats -fno-dfg']) -test.file_grep(test.stats, r'Optimizations, expand limited\s+(\d+)', 8) +test.file_grep(test.stats, r'Optimizations, expand limited\s+(\d+)', 7) test.passes()