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: