From 098fe9664383d9480396cfa1edf999e4cc8b3f61 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 28 Feb 2026 22:20:09 +0000 Subject: [PATCH] Add V3LiftExpr pass to lower impure expressions and calls (#7141) Introduce new pass that converts impure expressions, or those with function and method calls into simple assignment statements. Please see the blurb at the top of the file why this is useful and how it works. In particular currently it enables more Dfg optimization as functions will be inlined without AstExprStmt. Ideally we should enforce this lowering is applied to every procedural statement (there are still a handful of exceptions). With that, long term with this pass + #6820, there should be no need to ever use an AstExprStmt past this new lowering pass, which should enable more easier optimization down the line. Also ideally this should be run earlier. Currently it's after V3Tristate as that calls pinReconnectSimple so we don't have to touch Cell ports. Currently disabled when code coverage is enabled due to #7119. --- docs/gen/ex_ASSIGNEQEXPR_faulty.rst | 11 +- docs/guide/exe_verilator.rst | 2 + src/CMakeLists.txt | 2 + src/Makefile_obj.in | 1 + src/V3AstNodeExpr.h | 1 + src/V3AstNodes.cpp | 20 + src/V3Const.cpp | 6 +- src/V3Dead.cpp | 1 + src/V3LiftExpr.cpp | 492 ++++++++++++++++++++ src/V3LiftExpr.h | 32 ++ src/V3Options.cpp | 1 + src/V3Options.h | 2 + src/Verilator.cpp | 8 + test_regress/t/t_dfg_peephole.py | 1 + test_regress/t/t_flag_csplit_groups.py | 2 +- test_regress/t/t_lift_expr.py | 18 + test_regress/t/t_lift_expr.v | 37 ++ test_regress/t/t_lint_assigneqexpr.v | 7 +- test_regress/t/t_lint_assigneqexpr_bad.out | 8 +- test_regress/t/t_lint_assigneqexpr_bad.py | 2 +- test_regress/t/t_unopt_combo_isolate.py | 4 +- test_regress/t/t_unopt_combo_isolate_vlt.py | 3 +- 22 files changed, 645 insertions(+), 16 deletions(-) create mode 100644 src/V3LiftExpr.cpp create mode 100644 src/V3LiftExpr.h create mode 100755 test_regress/t/t_lift_expr.py create mode 100644 test_regress/t/t_lift_expr.v diff --git a/docs/gen/ex_ASSIGNEQEXPR_faulty.rst b/docs/gen/ex_ASSIGNEQEXPR_faulty.rst index af5f59067..948c309d1 100644 --- a/docs/gen/ex_ASSIGNEQEXPR_faulty.rst +++ b/docs/gen/ex_ASSIGNEQEXPR_faulty.rst @@ -1,8 +1,11 @@ .. comment: generated by t_lint_assigneqexpr_bad .. code-block:: sv :linenos: + :emphasize-lines: 3,5 - output logic c_o, - output logic d_o - ); - assign c_o = (a_i != 0) ? 1 : 0; + assign d_o = // Note = not == below + ( + e_o = 1 // <--- Warning: ASSIGNEQEXPR + ) ? 1 : ( + e_o = 0 // <--- Warning: ASSIGNEQEXPR + ) ? b_i : 0; diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index e3d434fa4..791d9417f 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -694,6 +694,8 @@ Summary: .. option:: -fno-life-post +.. option:: -fno-lift-expr + .. option:: -fno-localize .. option:: -fno-merge-cond diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7efc365d6..7c0612dee 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -120,6 +120,7 @@ set(HEADERS V3LanguageWords.h V3Life.h V3LifePost.h + V3LiftExpr.h V3LinkCells.h V3LinkDot.h V3LinkDotIfaceCapture.h @@ -294,6 +295,7 @@ set(COMMON_SOURCES V3LibMap.cpp V3Life.cpp V3LifePost.cpp + V3LiftExpr.cpp V3LinkCells.cpp V3LinkDot.cpp V3LinkDotIfaceCapture.cpp diff --git a/src/Makefile_obj.in b/src/Makefile_obj.in index cb80e1dac..506d8047e 100644 --- a/src/Makefile_obj.in +++ b/src/Makefile_obj.in @@ -290,6 +290,7 @@ RAW_OBJS_PCH_ASTNOMT = \ V3LibMap.o \ V3Life.o \ V3LifePost.o \ + V3LiftExpr.o \ V3LinkCells.o \ V3LinkDot.o \ V3LinkDotIfaceCapture.o \ diff --git a/src/V3AstNodeExpr.h b/src/V3AstNodeExpr.h index cbf596680..1d0f81148 100644 --- a/src/V3AstNodeExpr.h +++ b/src/V3AstNodeExpr.h @@ -64,6 +64,7 @@ public: // Someday we will generically support data types on every expr node // Until then isOpaque indicates we shouldn't constant optimize this node type bool isOpaque() const { return VN_IS(this, CvtPackString); } + bool isLValue() const; // Wrap This expression into an AstStmtExpr to denote it occurs in statement position inline AstStmtExpr* makeStmt(); diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 7c98ab18e..de9dd6539 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -1791,6 +1791,26 @@ string AstBasicDType::prettyDTypeName(bool) const { void AstNodeExpr::dump(std::ostream& str) const { this->AstNode::dump(str); } void AstNodeExpr::dumpJson(std::ostream& str) const { dumpJsonGen(str); } + +bool AstNodeExpr::isLValue() const { + if (const AstNodeVarRef* const varrefp = VN_CAST(this, NodeVarRef)) { + return varrefp->access().isWriteOrRW(); + } else if (const AstMemberSel* const memberselp = VN_CAST(this, MemberSel)) { + return memberselp->access().isWriteOrRW(); + } else if (const AstSel* const selp = VN_CAST(this, Sel)) { + return selp->fromp()->isLValue(); + } else if (const AstNodeSel* const nodeSelp = VN_CAST(this, NodeSel)) { + return nodeSelp->fromp()->isLValue(); + } else if (const AstConcat* const concatp = VN_CAST(this, Concat)) { + // Enough to check only one side, as both must be same otherwise malformed + return concatp->lhsp()->isLValue(); + } else if (const AstCMethodHard* const cMethodHardp = VN_CAST(this, CMethodHard)) { + // Used for things like Queue/AssocArray/DynArray + return cMethodHardp->fromp()->isLValue(); + } + return false; +} + void AstNodeUniop::dump(std::ostream& str) const { this->AstNodeExpr::dump(str); } void AstNodeUniop::dumpJson(std::ostream& str) const { dumpJsonGen(str); } diff --git a/src/V3Const.cpp b/src/V3Const.cpp index a57f795bb..674ca1530 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -1623,7 +1623,11 @@ class ConstVisitor final : public VNVisitor { if (!thensp->rhsp()->gateTree()) return false; if (!elsesp->rhsp()->gateTree()) return false; if (m_underRecFunc) return false; // This optimization may lead to infinite recursion - return true; + // Only do it if not calls and both pure, otherwise undoes V3LiftExpr + return !VN_IS(thensp->rhsp(), NodeFTaskRef) // + && !VN_IS(elsesp->rhsp(), NodeFTaskRef) // + && thensp->rhsp()->isPure() // + && elsesp->rhsp()->isPure(); } bool operandIfIf(const AstNodeIf* nodep) { if (nodep->elsesp()) return false; diff --git a/src/V3Dead.cpp b/src/V3Dead.cpp index d5392d69f..94dfbface 100644 --- a/src/V3Dead.cpp +++ b/src/V3Dead.cpp @@ -173,6 +173,7 @@ class DeadVisitor final : public VNVisitor { } void visit(AstNodeFTaskRef* nodep) override { iterateChildren(nodep); + if (!m_sideEffect && !nodep->isPure()) m_sideEffect = true; checkAll(nodep); if (nodep->taskp()) nodep->taskp()->user1Inc(); if (nodep->classOrPackagep()) { diff --git a/src/V3LiftExpr.cpp b/src/V3LiftExpr.cpp new file mode 100644 index 000000000..544ab93cd --- /dev/null +++ b/src/V3LiftExpr.cpp @@ -0,0 +1,492 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// DESCRIPTION: Verilator: Lift expressions out of statements +// +// Code available from: https://verilator.org +// +//************************************************************************* +// +// 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-FileCopyrightText: 2003-2026 Wilson Snyder +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 +// +//************************************************************************* +// +// V3LiftExpr's Transformations: +// +// Lift impure sub-expressions and function calls out of expressions, +// turning them into additional statements. This has several benefits +// that enable further downstream optimizations: +// - Impure expressions always appear on the RHS of a simple assignment, +// reducing the size of impure expressions. This also eliminates +// later needs for cloning impure expressions, preserving side effects +// - Lifted function calls can be inlined without the use of AstExprStmt, +// which is poorly handled by optimizations, especially Dfg +// - Reduces complexity of downstream lowering passes as they need to deal +// with fewer special cases. +// +// The generic transformation applies for all AstNodeStmt. Using AstAssign +// as an example: +// x[impure_x] = impure_y + func(impure_z); +// is transformed into: +// __VleImpure_0 = impure_y; +// __VleImpure_1 = impure_z; +// __VleCall_0 = func(__VleImpure_1); +// __VleImpure_2 = impure_x; +// x[__VleImpure_2] = __VleImpure_0 + __VleCall_0; +// All parts of the assignment is now pure, and the function call can be +// inlined by V3Task without the use of AstExprStmt. +// +// Care must be taken for the 4 short-circuiting operators: && || -> ?: +// For example AstLogAnd: +// z = x && func(y) +// is transformed into: +// __VleLogAnd_0 = x; +// if (__VleLogAnd_0) { +// __VleCall_0 = func(y); +// __VleLogAnd_0 = __VleCall_0; +// } +// z = __VleLogAnd_1; +// Similar patterns are used for AstLogOr and AstCond to preserve the +// short-circuiting semantics and side effects. All AstLogIf should have +// been converted to AstLogOr earlier by V3Const. +// +// Care must be taken for impure LValues as well. While, all LValue expressions +// permitted by IEEE-1800 are themselves pure (except possibly for the non-lvalue +// sub-expressions like array indices, which are not a problem), we support a +// non-standard but common extension where a function call returning a class +// handle, under a member select can be an LValue, e.g.: +// 'getInstance().member = foo;' +// Fortunately we can discover these via MemberSel, and we can transform them +// to the IEEE-1800 compliant form: +// __VleLvalCall_0 = getInstance(); +// __VleLvalCall_0.member = foo; +// There are also some other internal LValues represented via CMethodHard that +// return references, and are marked as impure. For this reason only we still +// need to special case their handling via AstNodeExpr::isLValue(). +// +//************************************************************************* + +#include "V3PchAstNoMT.h" // VL_MT_DISABLED_CODE_UNIT + +#include "verilatedos.h" + +#include "V3LiftExpr.h" + +#include "V3Ast.h" +#include "V3Error.h" +#include "V3FileLine.h" +#include "V3Inst.h" +#include "V3Stats.h" + +VL_DEFINE_DEBUG_FUNCTIONS; + +//###################################################################### + +class LiftExprVisitor final : public VNVisitor { + // NODE STATE + // AstNodeStmt::user1() -> bool. Statement already processed + // AstVar::user1() -> bool. Variable is a lifted temporary + // AstNodeExpr::user1p() -> AstVar*. Existing temporary variable usable for this expression + const VNUser1InUse m_user1InUse; + + // STATE + AstNodeModule* m_modp = nullptr; // Current module + AstNodeFTask* m_ftaskp = nullptr; // Current function/task + bool m_lift = false; // Lift encountered expressions + // Statements lifted out of current node. TODO: Make this an AstNodeStmt* after #6280 + AstNode* m_newStmtps = nullptr; + // Expressions in some special locations need not, or should not be lifted + AstNodeExpr* m_doNotLiftp = nullptr; + size_t m_nTmps = 0; // Sequence numbers for temporary variables + // Statistics + VDouble0 m_statLiftedExprs; + VDouble0 m_statLiftedCalls; + VDouble0 m_statLiftedLvalCalls; + VDouble0 m_statLiftedConds; + VDouble0 m_statLiftedLogAnds; + VDouble0 m_statLiftedLogOrs; + VDouble0 m_statLiftedExprStmts; + VDouble0 m_statTemporariesCreated; + VDouble0 m_statTemporariesReused; + + // METHODS + AstVar* newVar(const char* baseName, const AstNodeExpr* exprp, + const std::string& suffix = "") { + // Reuse existing temporary if available + if (exprp->user1p()) { + ++m_statTemporariesReused; + return VN_AS(exprp->user1p(), Var); + } + + // Need a separate name in functions vs modules, as module inlining + // can otherwise cause spurious VARHIDDEN warnings + std::string name = m_ftaskp ? "__Vlef" : "__Vlem"; + name += baseName; + name += "_" + std::to_string(m_nTmps++); + if (!suffix.empty()) name += "__" + suffix; + + // Create variable + ++m_statTemporariesCreated; + AstVar* const varp = new AstVar{exprp->fileline(), VVarType::MODULETEMP, name, + exprp->dtypep()->skipRefp()}; + varp->isInternal(true); + varp->noReset(true); + varp->user1(1); // Mark as lifted temporary so it can be reused + + if (m_ftaskp) { + varp->funcLocal(true); + m_ftaskp->stmtsp()->addHereThisAsNext(varp); + varp->lifetime(VLifetime::AUTOMATIC_EXPLICIT); + } else { + m_modp->stmtsp()->addHereThisAsNext(varp); + // 'automatic' on a variable inside a class actually means 'member' + varp->lifetime(VN_IS(m_modp, Class) ? VLifetime::STATIC_EXPLICIT + : VLifetime::AUTOMATIC_EXPLICIT); + } + return varp; + } + + // If the expression is a reference to an existing temporary, return it, otherwise nullptr + AstVar* getExistingVar(AstNodeExpr* nodep) { + if (AstVarRef* const refp = VN_CAST(nodep, VarRef)) { + if (refp->varp()->user1()) return refp->varp(); + } + return nullptr; + } + + // Unlink and assign given expression to temporary, unless the expression + // is a reference to the same temporary + AstAssign* assignIfDifferent(FileLine* flp, AstVar* varp, AstNodeExpr* nodep) { + if (AstVarRef* const refp = VN_CAST(nodep, VarRef)) { + if (refp->varp() == varp) return nullptr; + } + return new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, nodep->unlinkFrBack()}; + } + + void addStmtps(AstNode* stmtp) { + if (!stmtp) return; + // No need to process again, so mark + for (AstNode* nodep = stmtp; nodep; nodep = nodep->nextp()) nodep->user1(1); + m_newStmtps = AstNode::addNext(m_newStmtps, stmtp); + } + + // Lift expressions from expression, return lifted statements + AstNode* lift(AstNodeExpr* nodep) { + if (!nodep) return nullptr; + VL_RESTORER(m_lift); + VL_RESTORER(m_newStmtps); + m_lift = true; + m_newStmtps = nullptr; + iterate(nodep); + return m_newStmtps; + } + + // Lift expressions from children of given statement, return lifted statements + AstNode* liftChildren(AstNodeStmt* nodep) { + VL_RESTORER(m_lift); + VL_RESTORER(m_newStmtps); + m_lift = true; + m_newStmtps = nullptr; + iterateChildren(nodep); + return m_newStmtps; + } + + // VISITORS - non-statement, non-expression + void visit(AstNode* nodep) override { + VL_RESTORER(m_lift); + m_lift = false; // Conservatively do not lift if unknown construct + iterateChildren(nodep); + } + void visit(AstNodeModule* nodep) override { + // Reset names on root module only (there can be classes nested in modules) + if (!m_modp) m_nTmps = 0; + VL_RESTORER(m_modp); + m_modp = nodep; + iterateChildren(nodep); + } + void visit(AstNodeFTask* nodep) override { + VL_RESTORER(m_ftaskp); + VL_RESTORER(m_nTmps); + m_ftaskp = nodep; + m_nTmps = 0; + iterateChildren(nodep); + } + void visit(AstCaseItem* nodep) override { + // Do not lift from the case expressions, there is nowhere to put them + iterateAndNextNull(nodep->stmtsp()); + } + void visit(AstCell* nodep) override { + // No need to fix up port connections, V3Tristate called pinReconnectSimple, + // so all writes are to simple AstVars, assuming it did it right ... + } + void visit(AstAlias* nodep) override {} + void visit(AstArg* nodep) override { + // Lift argument expressions + iterateChildren(nodep); + } + + // VISITORS - statements + void visit(AstNodeStmt* nodep) override { + if (nodep->user1SetOnce()) return; + VL_RESTORER(m_doNotLiftp); + m_doNotLiftp = nullptr; + if (AstNode* const newStmtps = liftChildren(nodep)) nodep->addHereThisAsNext(newStmtps); + } + void visit(AstNodeAssign* nodep) override { + if (nodep->user1SetOnce()) return; + VL_RESTORER(m_doNotLiftp); + // Do not lift the RHS if this is already a simple assignment to a variable + m_doNotLiftp = VN_IS(nodep->lhsp(), NodeVarRef) ? nodep->rhsp() : nullptr; + if (AstNode* const newStmtps = lift(nodep->rhsp())) { + nodep->addHereThisAsNext(newStmtps); + } + + if (nodep->timingControlp()) return; + + if (AstNode* const newStmtps = lift(nodep->lhsp())) { + nodep->addHereThisAsNext(newStmtps); + } + } + void visit(AstStmtExpr* nodep) override { + if (nodep->user1SetOnce()) return; + // Ignore super class constructor calls - can't insert statements before them + if (VN_IS(nodep->exprp(), New)) return; + VL_RESTORER(m_doNotLiftp); + // Do not lift if the expression itself. This AstStmtExpr is required to + // throw away the return value if any, and V3Task can inline without using + // AstExprStmt in this case. Can still lift all sub-expressions though. + m_doNotLiftp = nodep->exprp(); + if (AstNode* const newStmtps = liftChildren(nodep)) nodep->addHereThisAsNext(newStmtps); + } + // Don't know whether these are sensitive to lifting, assume so + void visit(AstCStmt* nodep) override {} + void visit(AstCStmtUser* nodep) override {} + + // VISITORS - expressions + void visit(AstNodeExpr* nodep) override { + if (!m_lift) return; + + iterateChildren(nodep); + + // Do not lift if already in normal form + if (m_doNotLiftp == nodep) return; + // No need to lift void expressions, these should be under StmtExpr, but just in case ... + if (VN_IS(nodep->dtypep()->skipRefp(), VoidDType)) return; + // Do not lift if pure + if (nodep->isPure()) return; + // Do not lift if LValue + if (nodep->isLValue()) return; + + // Extract expression into a temporary variable + ++m_statLiftedExprs; + FileLine* const flp = nodep->fileline(); + AstVar* const varp = newVar("Expr", nodep); + nodep->replaceWith(new AstVarRef{flp, varp, VAccess::READ}); + addStmtps(new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, nodep}); + } + void visit(AstNodeFTaskRef* nodep) override { + if (!m_lift) return; + + iterateChildren(nodep); + + // Do not lift if already in normal form + if (m_doNotLiftp == nodep) return; + // No need to lift void functions, these should be under StmtExpr, but just in case ... + if (VN_IS(nodep->dtypep()->skipRefp(), VoidDType)) return; + // Do not lift Taskref, it's always in statement position and cleanly inlineable. + if (VN_IS(nodep, TaskRef)) return; + + // Extract expression into a temporary variable + ++m_statLiftedCalls; + FileLine* const flp = nodep->fileline(); + AstVar* const varp = newVar("Call", nodep, nodep->taskp()->name()); + nodep->replaceWith(new AstVarRef{flp, varp, VAccess::READ}); + addStmtps(new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, nodep}); + } + void visit(AstMemberSel* nodep) override { + if (!m_lift) return; + // MemberSel is special as it can appear as an LValue selecting from a call + // that returns a class handle. Note this is not in IEEE-1800, but is supported + // for compatibility. Here we turn it into an IEEE-1800 compliant LValue by + // lifting the call and use the return value via a temporary variable. + + VL_RESTORER(m_doNotLiftp); + // If it's an LValue call, do not lift the fromp, will do it explicitly below + m_doNotLiftp = nodep->access().isWriteOrRW() && VN_IS(nodep->fromp(), NodeFTaskRef) + ? nodep->fromp() + : nullptr; + iterateChildren(nodep); + // Done if not a special case + if (!m_doNotLiftp) return; + + // Extract LValue call into a temporary variable + ++m_statLiftedLvalCalls; + FileLine* const flp = nodep->fileline(); + AstVar* const varp = newVar("LvalCall", nodep->fromp()); + addStmtps(new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, + nodep->fromp()->unlinkFrBack()}); + // This one is a WRITE or READWRITE, same as the MemberSel + nodep->fromp(new AstVarRef{flp, varp, nodep->access()}); + } + + // VISITORS - RValue expressions + void visit(AstCond* nodep) override { + if (!m_lift) return; + + // Lift from condition + iterate(nodep->condp()); + + // Temporary variable to use for this expression + AstVar* varp = nullptr; + + const bool sameType + = nodep->thenp()->dtypep()->skipRefp()->sameTree(nodep->elsep()->dtypep()->skipRefp()); + + // Lift from the Then branch + AstNode* const thenStmtps = lift(nodep->thenp()); + // If lifted the Then branch, we can reuse the temporary if same type + if (sameType) varp = getExistingVar(nodep->thenp()); + if (varp) nodep->elsep()->user1p(varp); + + // Lift from the Else branch + AstNode* const elseStmtps = lift(nodep->elsep()); + // If lifted the Else branch, we can reuse the temporary if same type + if (sameType && !varp) varp = getExistingVar(nodep->elsep()); + + // If nothing lifted, then nothing to do + if (!thenStmtps && !elseStmtps) return; + + // Otherwise convert to an AstIf with a temporary variable + ++m_statLiftedConds; + FileLine* const flp = nodep->fileline(); + if (!varp) varp = newVar("Cond", nodep); + // Create the AstIf + AstIf* const ifp = new AstIf{flp, nodep->condp()->unlinkFrBack()}; + addStmtps(ifp); + ifp->addThensp(thenStmtps); + ifp->addThensp(assignIfDifferent(flp, varp, nodep->thenp())); + ifp->addElsesp(elseStmtps); + ifp->addElsesp(assignIfDifferent(flp, varp, nodep->elsep())); + // Replace the expression with a reference to the temporary variable + nodep->replaceWith(new AstVarRef{flp, varp, VAccess::READ}); + VL_DO_DANGLING(nodep->deleteTree(), nodep); + } + void visit(AstLogAnd* nodep) override { + if (!m_lift) return; + + // Lift from LHS + iterate(nodep->lhsp()); + + // Temporary variable to use for this expression + AstVar* varp = getExistingVar(nodep->lhsp()); + // If lifted the LHS, we can reuse the temporary + if (varp) nodep->rhsp()->user1p(varp); + + // Lift from RHS, if nothing lifted, then nothing to do + AstNode* const rhsStmtps = lift(nodep->rhsp()); + if (!rhsStmtps) return; + + // Otherwise convert to an AstIf with a temporary variable + ++m_statLiftedLogAnds; + FileLine* const flp = nodep->fileline(); + if (!varp) varp = getExistingVar(nodep->rhsp()); + if (!varp) varp = newVar("LogAnd", nodep); + addStmtps(assignIfDifferent(flp, varp, nodep->lhsp())); + AstIf* const ifp = new AstIf{flp, new AstVarRef{flp, varp, VAccess::READ}}; + addStmtps(ifp); + ifp->addThensp(rhsStmtps); + ifp->addThensp(assignIfDifferent(flp, varp, nodep->rhsp())); + nodep->replaceWith(new AstVarRef{flp, varp, VAccess::READ}); + VL_DO_DANGLING(nodep->deleteTree(), nodep); + } + void visit(AstLogOr* nodep) override { + if (!m_lift) return; + + // Lift from LHS + iterate(nodep->lhsp()); + + // Temporary variable to use for this expression + AstVar* varp = getExistingVar(nodep->lhsp()); + // If lifted the LHS, we can reuse the temporary + if (varp) nodep->rhsp()->user1p(varp); + + // Lift from RHS, if nothing lifted, then nothing to do + AstNode* const rhsStmtps = lift(nodep->rhsp()); + if (!rhsStmtps) return; + + // Otherwise convert to an AstIf with a temporary variable + ++m_statLiftedLogOrs; + FileLine* const flp = nodep->fileline(); + if (!varp) varp = getExistingVar(nodep->rhsp()); + if (!varp) varp = newVar("LogOr", nodep); + addStmtps(assignIfDifferent(flp, varp, nodep->lhsp())); + AstIf* const ifp = new AstIf{flp, new AstVarRef{flp, varp, VAccess::READ}}; + addStmtps(ifp); + ifp->addElsesp(rhsStmtps); + ifp->addElsesp(assignIfDifferent(flp, varp, nodep->rhsp())); + nodep->replaceWith(new AstVarRef{flp, varp, VAccess::READ}); + VL_DO_DANGLING(nodep->deleteTree(), nodep); + } + void visit(AstLogIf* nodep) override { + if (!m_lift) return; + nodep->v3fatalSrc("AstLogIf should have been folded by V3Const"); + } + void visit(AstExprStmt* nodep) override { + if (!m_lift) return; + + // Eliminate AstExprStmt by lifting the content entirely + ++m_statLiftedExprStmts; + iterate(nodep->stmtsp()); + addStmtps(nodep->stmtsp()->unlinkFrBackWithNext()); + iterate(nodep->resultp()); + nodep->replaceWith(nodep->resultp()->unlinkFrBack()); + VL_DO_DANGLING(nodep->deleteTree(), nodep); + } + + // VISITORS - Accelerate pure leaf expressions + void visit(AstConst*) override {} + void visit(AstVarRef*) override {} + void visit(AstVarXRef*) override {} + + // VISITORS - Expression special cases + // These return C++ references rather than values, cannot be lifted + void visit(AstAssocSel* nodep) override { iterateChildren(nodep); } + void visit(AstCMethodHard* nodep) override { iterateChildren(nodep); } + // Don't know whether these may return non-values, assume so + void visit(AstCExpr* nodep) override { iterateChildren(nodep); } + void visit(AstCExprUser* nodep) override { iterateChildren(nodep); } + +public: + // CONSTRUCTORS + explicit LiftExprVisitor(AstNetlist* nodep) { + // Extracting expressions can effect purity + VIsCached::clearCacheTree(); + iterate(nodep); + VIsCached::clearCacheTree(); + if (m_newStmtps) m_newStmtps->dumpTreeAndNext(std::cout, "Leftover:"); + UASSERT_OBJ(!m_newStmtps, m_newStmtps, "Failed to insert statements"); + } + ~LiftExprVisitor() override { + V3Stats::addStat("LiftExpr, lifted impure expressions", m_statLiftedExprs); + V3Stats::addStat("LiftExpr, lifted calls", m_statLiftedCalls); + V3Stats::addStat("LiftExpr, lifted calls as lvalue", m_statLiftedLvalCalls); + V3Stats::addStat("LiftExpr, lifted Cond", m_statLiftedConds); + V3Stats::addStat("LiftExpr, lifted LogAnd", m_statLiftedLogAnds); + V3Stats::addStat("LiftExpr, lifted LogOr", m_statLiftedLogOrs); + V3Stats::addStat("LiftExpr, lifted ExprStmt", m_statLiftedExprStmts); + V3Stats::addStat("LiftExpr, temporaries created", m_statTemporariesCreated); + V3Stats::addStat("LiftExpr, temporaries reused", m_statTemporariesReused); + } +}; + +//###################################################################### +// Unknown class functions + +void V3LiftExpr::liftExprAll(AstNetlist* nodep) { + UINFO(2, __FUNCTION__ << ":"); + LiftExprVisitor{nodep}; + V3Global::dumpCheckGlobalTree("lift_expr", 0, dumpTreeEitherLevel() >= 3); +} diff --git a/src/V3LiftExpr.h b/src/V3LiftExpr.h new file mode 100644 index 000000000..250a39e68 --- /dev/null +++ b/src/V3LiftExpr.h @@ -0,0 +1,32 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// DESCRIPTION: Verilator: Lift expressions out of statements +// +// Code available from: https://verilator.org +// +//************************************************************************* +// +// 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-FileCopyrightText: 2003-2026 Wilson Snyder +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 +// +//************************************************************************* + +#ifndef VERILATOR_V3LIFTEXPR_H_ +#define VERILATOR_V3LIFTEXPR_H_ + +#include "config_build.h" +#include "verilatedos.h" + +class AstNetlist; + +//============================================================================ + +class V3LiftExpr final { +public: + static void liftExprAll(AstNetlist* nodep) VL_MT_DISABLED; +}; + +#endif // Guard diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 246aee89e..6992d0870 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -1480,6 +1480,7 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, DECL_OPTION("-finline-funcs-eager", FOnOff, &m_fInlineFuncsEager); DECL_OPTION("-flife", FOnOff, &m_fLife); DECL_OPTION("-flife-post", FOnOff, &m_fLifePost); + DECL_OPTION("-flift-expr", FOnOff, &m_fLiftExpr); DECL_OPTION("-flocalize", FOnOff, &m_fLocalize); DECL_OPTION("-fmerge-cond", FOnOff, &m_fMergeCond); DECL_OPTION("-fmerge-cond-motion", FOnOff, &m_fMergeCondMotion); diff --git a/src/V3Options.h b/src/V3Options.h index 08df2599d..6340490b7 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -413,6 +413,7 @@ private: bool m_fInlineFuncsEager = true; // main switch: -fno-inline-funcs-eager: don't inline eagerly bool m_fLife; // main switch: -fno-life: variable lifetime bool m_fLifePost; // main switch: -fno-life-post: delayed assignment elimination + bool m_fLiftExpr = true; // main switch: -fno-lift-expr: lift expressions out of statements bool m_fLocalize; // main switch: -fno-localize: convert temps to local variables bool m_fMergeCond; // main switch: -fno-merge-cond: merge conditionals bool m_fMergeCondMotion = true; // main switch: -fno-merge-cond-motion: perform code motion @@ -734,6 +735,7 @@ public: bool fInlineFuncsEager() const { return m_fInlineFuncsEager; } bool fLife() const { return m_fLife; } bool fLifePost() const { return m_fLifePost; } + bool fLiftExpr() const { return m_fLiftExpr; } bool fLocalize() const { return m_fLocalize; } bool fMergeCond() const { return m_fMergeCond; } bool fMergeCondMotion() const { return m_fMergeCondMotion; } diff --git a/src/Verilator.cpp b/src/Verilator.cpp index c3b90208f..8d4ef283d 100644 --- a/src/Verilator.cpp +++ b/src/Verilator.cpp @@ -67,6 +67,7 @@ #include "V3LibMap.h" #include "V3Life.h" #include "V3LifePost.h" +#include "V3LiftExpr.h" #include "V3LinkDot.h" #include "V3LinkInc.h" #include "V3LinkJump.h" @@ -284,6 +285,13 @@ static void process() { } if (!v3Global.opt.serializeOnly()) { + // Lift expressions out of statements. Currently disabled for line and + // expression coverage, as otherwise later V3Split would further split + // combinational always blocks and alter counts (that needs to be fixed in V3Split) + if (v3Global.opt.fLiftExpr() // + && !v3Global.opt.coverageLine() && !v3Global.opt.coverageExpr()) { + V3LiftExpr::liftExprAll(v3Global.rootp()); + } // Move assignments from X into MODULE temps. // (Before flattening, so each new X variable is shared between all scopes of that // module.) diff --git a/test_regress/t/t_dfg_peephole.py b/test_regress/t/t_dfg_peephole.py index 71833c22c..b527d345d 100755 --- a/test_regress/t/t_dfg_peephole.py +++ b/test_regress/t/t_dfg_peephole.py @@ -91,6 +91,7 @@ test.compile(verilator_flags2=[ "-Mdir", test.obj_dir + "/obj_opt", "--prefix", "Vopt", "-fno-const-before-dfg", # Otherwise V3Const makes testing painful + "-fno-lift-expr", # Assumes V3Const run prior to V3LiftExpr "-fdfg-synthesize-all", "--dump-dfg", # To fill code coverage "-CFLAGS \"-I .. -I ../obj_ref\"", diff --git a/test_regress/t/t_flag_csplit_groups.py b/test_regress/t/t_flag_csplit_groups.py index 6e15ff128..c92448c9e 100755 --- a/test_regress/t/t_flag_csplit_groups.py +++ b/test_regress/t/t_flag_csplit_groups.py @@ -125,7 +125,7 @@ test.file_grep_not(test.obj_dir + "/" + test.vm_prefix + "_classes.mk", "vm_clas test.file_grep_not(test.obj_dir + "/" + test.vm_prefix + "_classes.mk", "vm_classes_2") # Check combine count -test.file_grep(test.stats, r'Node count, CFILE + (\d+)', (275 if test.vltmt else 258)) +test.file_grep(test.stats, r'Node count, CFILE + (\d+)', (276 if test.vltmt else 259)) test.file_grep(test.stats, r'Makefile targets, VM_CLASSES_FAST + (\d+)', 2) test.file_grep(test.stats, r'Makefile targets, VM_CLASSES_SLOW + (\d+)', 2) diff --git a/test_regress/t/t_lift_expr.py b/test_regress/t/t_lift_expr.py new file mode 100755 index 000000000..2351d6963 --- /dev/null +++ b/test_regress/t/t_lift_expr.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# 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-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('vlt') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_lift_expr.v b/test_regress/t/t_lift_expr.v new file mode 100644 index 000000000..2b7b41b52 --- /dev/null +++ b/test_regress/t/t_lift_expr.v @@ -0,0 +1,37 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// 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-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +// verilog_format: off +`define stop $stop +`define checkh(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0x exp=%0x (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); +// verilog_format: on + +module t; + + function automatic int one(); + return 1; + endfunction + + function automatic int two(); + /* verilator no_inline_task */ + return 2; + endfunction + + class C; + static int i = one() + 1; + static int j = two() + 1; + endclass + + initial begin + `checkh(C::i, 2); + `checkh(C::j, 3); + $write("*-* All Finished *-*\n"); + $finish; + end + +endmodule diff --git a/test_regress/t/t_lint_assigneqexpr.v b/test_regress/t/t_lint_assigneqexpr.v index 59420699e..fc7fefaf1 100644 --- a/test_regress/t/t_lint_assigneqexpr.v +++ b/test_regress/t/t_lint_assigneqexpr.v @@ -24,13 +24,14 @@ module Sub ( input logic [2:0] a_i, input logic b_i, output logic c_o, - output logic d_o + output logic d_o, + output logic e_o ); assign c_o = (a_i != 0) ? 1 : 0; assign d_o = // Note = not == below ( - c_o = 1 // <--- Warning: ASSIGNEQEXPR + e_o = 1 // <--- Warning: ASSIGNEQEXPR ) ? 1 : ( - c_o = 0 // <--- Warning: ASSIGNEQEXPR + e_o = 0 // <--- Warning: ASSIGNEQEXPR ) ? b_i : 0; endmodule diff --git a/test_regress/t/t_lint_assigneqexpr_bad.out b/test_regress/t/t_lint_assigneqexpr_bad.out index 0b2ab275f..79d777aca 100644 --- a/test_regress/t/t_lint_assigneqexpr_bad.out +++ b/test_regress/t/t_lint_assigneqexpr_bad.out @@ -1,11 +1,11 @@ -%Warning-ASSIGNEQEXPR: t/t_lint_assigneqexpr.v:32:11: Assignment '=' inside expression +%Warning-ASSIGNEQEXPR: t/t_lint_assigneqexpr.v:33:11: Assignment '=' inside expression : ... Was a '==' intended, or suggest use a separate statement - 32 | c_o = 1 + 33 | e_o = 1 | ^ ... For warning description see https://verilator.org/warn/ASSIGNEQEXPR?v=latest ... Use "/* verilator lint_off ASSIGNEQEXPR */" and lint_on around source to disable this message. -%Warning-ASSIGNEQEXPR: t/t_lint_assigneqexpr.v:34:11: Assignment '=' inside expression +%Warning-ASSIGNEQEXPR: t/t_lint_assigneqexpr.v:35:11: Assignment '=' inside expression : ... Was a '==' intended, or suggest use a separate statement - 34 | c_o = 0 + 35 | e_o = 0 | ^ %Error: Exiting due to diff --git a/test_regress/t/t_lint_assigneqexpr_bad.py b/test_regress/t/t_lint_assigneqexpr_bad.py index 7f3535d78..6b8b54ad2 100755 --- a/test_regress/t/t_lint_assigneqexpr_bad.py +++ b/test_regress/t/t_lint_assigneqexpr_bad.py @@ -18,7 +18,7 @@ test.lint(verilator_flags2=['-Wall -Wno-DECLFILENAME'], test.extract(in_filename=test.top_filename, out_filename=test.root + "/docs/gen/ex_ASSIGNEQEXPR_faulty.rst", - lines="26-29") + lines="31-36") test.extract(in_filename=test.golden_filename, out_filename=test.root + "/docs/gen/ex_ASSIGNEQEXPR_msg.rst", diff --git a/test_regress/t/t_unopt_combo_isolate.py b/test_regress/t/t_unopt_combo_isolate.py index f35574a26..f7b4618d3 100755 --- a/test_regress/t/t_unopt_combo_isolate.py +++ b/test_regress/t/t_unopt_combo_isolate.py @@ -14,7 +14,9 @@ test.top_filename = "t/t_unopt_combo.v" out_filename = test.obj_dir + "/V" + test.name + ".tree.json" -test.compile(verilator_flags2=["--no-json-edit-nums", "+define+ISOLATE", "--stats", "-fno-dfg"]) +test.compile(verilator_flags2=[ + "--no-json-edit-nums", "+define+ISOLATE", "--stats", "-fno-dfg", "-fno-lift-expr" +]) if test.vlt_all: test.file_grep(test.stats, r'Optimizations, isolate_assignments blocks\s+4') diff --git a/test_regress/t/t_unopt_combo_isolate_vlt.py b/test_regress/t/t_unopt_combo_isolate_vlt.py index 803550765..35e0cc8d2 100755 --- a/test_regress/t/t_unopt_combo_isolate_vlt.py +++ b/test_regress/t/t_unopt_combo_isolate_vlt.py @@ -15,7 +15,8 @@ test.top_filename = "t/t_unopt_combo.v" out_filename = test.obj_dir + "/V" + test.name + ".tree.json" test.compile(verilator_flags2=[ - "--no-json-edit-nums", "--stats", test.t_dir + "/t_unopt_combo_isolate.vlt", "-fno-dfg" + "--no-json-edit-nums", "--stats", test.t_dir + + "/t_unopt_combo_isolate.vlt", "-fno-dfg", "-fno-lift-expr" ]) if test.vlt_all: