From 573c2e61a63bd0cb5f29ca93fcdce98fd7f1eb63 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Wed, 16 Sep 2026 18:29:56 +0200 Subject: [PATCH] Internals: Make V3LiftExpr non optional (#8369) --- docs/guide/exe_verilator.rst | 4 ++ src/V3LiftExpr.cpp | 29 ++++++-- src/V3Options.cpp | 4 +- src/V3Options.h | 2 - src/Verilator.cpp | 2 +- .../t/t_covergroup_ref_bind_lvalue.py | 7 +- test_regress/t/t_covergroup_ref_bind_lvalue.v | 12 ++-- test_regress/t/t_dfg_peephole.py | 1 - test_regress/t/t_flag_deprecated_bad.out | 1 + test_regress/t/t_flag_deprecated_bad.py | 2 +- test_regress/t/t_opt_lift_expr.cpp | 17 +++++ test_regress/t/t_opt_lift_expr.py | 14 +++- test_regress/t/t_opt_lift_expr.v | 70 +++++++++++++++++++ 13 files changed, 141 insertions(+), 24 deletions(-) create mode 100644 test_regress/t/t_opt_lift_expr.cpp diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index ae1f9301d..a8f819c17 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -813,6 +813,10 @@ Summary: .. option:: -fno-lift-expr + Deprecated and has no effect (ignored). + + In versions before 5.054: Disable lifting of expressions out of statements. + .. option:: -fno-localize .. option:: -fno-merge-cond diff --git a/src/V3LiftExpr.cpp b/src/V3LiftExpr.cpp index d6bd9e7c2..94c927206 100644 --- a/src/V3LiftExpr.cpp +++ b/src/V3LiftExpr.cpp @@ -49,9 +49,8 @@ // __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. +// Similar patterns are used for AstLogOr, AstLogIf and AstCond to preserve +// the short-circuiting semantics and side effects. // // 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 @@ -108,6 +107,7 @@ class LiftExprVisitor final : public VNVisitor { VDouble0 m_statLiftedConds; VDouble0 m_statLiftedLogAnds; VDouble0 m_statLiftedLogOrs; + VDouble0 m_statLiftedLogIfs; VDouble0 m_statLiftedExprStmts; VDouble0 m_statTemporariesCreated; VDouble0 m_statTemporariesReused; @@ -432,7 +432,27 @@ class LiftExprVisitor final : public VNVisitor { } void visit(AstLogIf* nodep) override { if (!m_lift) return; - nodep->v3fatalSrc("AstLogIf should have been folded by V3Const"); + + // Lift from LHS + iterate(nodep->lhsp()); + + // 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_statLiftedLogIfs; + FileLine* const flp = nodep->fileline(); + AstVar* varp = getExistingVar(nodep->rhsp()); + if (!varp) varp = newVar("LogIf", nodep); + addStmtps(new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, + new AstLogNot{flp, nodep->lhsp()->unlinkFrBack()}}); + 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(AstExprStmt* nodep) override { if (!m_lift) return; @@ -476,6 +496,7 @@ public: 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 LogIf", m_statLiftedLogIfs); V3Stats::addStat("LiftExpr, lifted ExprStmt", m_statLiftedExprStmts); V3Stats::addStat("LiftExpr, temporaries created", m_statTemporariesCreated); V3Stats::addStat("LiftExpr, temporaries reused", m_statTemporariesReused); diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 2381410e9..018bd3bef 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -1518,7 +1518,9 @@ 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("-flift-expr", CbFOnOff, [fl](bool) { + fl->v3warn(DEPRECATED, "Option '-fno-lift-expr' is deprecated and has no effect"); + }); 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 c32f0827d..b02060f87 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -424,7 +424,6 @@ 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 @@ -765,7 +764,6 @@ 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 76dbe1be9..fc53162f1 100644 --- a/src/Verilator.cpp +++ b/src/Verilator.cpp @@ -307,7 +307,7 @@ static void process() { if (!v3Global.opt.serializeOnly()) { // Lift expressions out of statements. - if (v3Global.opt.fLiftExpr()) V3LiftExpr::liftExprAll(v3Global.rootp()); + 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 diff --git a/test_regress/t/t_covergroup_ref_bind_lvalue.py b/test_regress/t/t_covergroup_ref_bind_lvalue.py index b69d15553..a7956041c 100755 --- a/test_regress/t/t_covergroup_ref_bind_lvalue.py +++ b/test_regress/t/t_covergroup_ref_bind_lvalue.py @@ -17,13 +17,8 @@ test.scenarios('vlt_all') # it reliably. --no-threads-coarsen keeps the sample and the writer in separate MTasks. test.enable_tsan() -# -fno-lift-expr is what makes this test bite. With expression lifting on, V3LiftExpr rewrites -# 'arr[0] = new(sigs[0])' into '__VlemCall_0 = new(sigs[0]); arr[0] = __VlemCall_0', so the -# construction always assigns to a plain variable and the array element never reaches -# V3SchedCovergroup. With it off the raw form survives, and the binding is only seen at all -# because constructions are also collected where they are not a simple assignment. coverage_covergroup_common.run(test, - verilator_flags2=(['--stats', '-fno-lift-expr'] + + verilator_flags2=(['--stats'] + (['--no-threads-coarsen'] if test.vltmt else [])), threads=(2 if test.vltmt else 1)) diff --git a/test_regress/t/t_covergroup_ref_bind_lvalue.v b/test_regress/t/t_covergroup_ref_bind_lvalue.v index a1a52e2d6..e4faeced9 100644 --- a/test_regress/t/t_covergroup_ref_bind_lvalue.v +++ b/test_regress/t/t_covergroup_ref_bind_lvalue.v @@ -1,13 +1,11 @@ // DESCRIPTION: Verilator: Test covergroup 'ref' bindings where no handle is a plain variable // Companion to t_covergroup_ref_bind, which covers the resolvable shapes. Here nothing is a // plain variable: the covergroup is constructed into an array element, sampled through an array -// element, and the 'ref' actual is an array element too. So neither the construction nor the -// sample names one covergroup object, and both must fall back to the union over the covergroup -// type -- which must still order every sample against the non-blocking writer of what any -// instance of that type reads. cg_b repeats that with a struct member as the handle instead of -// an array element. Runs under --vltmt, where an unordered sample is a data race, -// and with -fno-lift-expr, which leaves the construction assigning directly to the array -// element instead of to a lifted temporary. +// element, and the 'ref' actual is an array element too. So no sample names one covergroup +// object, and each must fall back to the union over the covergroup type -- which must still +// order every sample against the non-blocking writer of what any instance of that type reads. +// cg_b repeats that with a struct member as the handle instead of an array element. Runs +// under --vltmt, where an unordered sample is a data race. // This file ONLY is placed into the Public Domain, for any use, without warranty. // SPDX-FileCopyrightText: 2026 Wilson Snyder // SPDX-License-Identifier: CC0-1.0 diff --git a/test_regress/t/t_dfg_peephole.py b/test_regress/t/t_dfg_peephole.py index 593b11b10..e803abdeb 100755 --- a/test_regress/t/t_dfg_peephole.py +++ b/test_regress/t/t_dfg_peephole.py @@ -94,7 +94,6 @@ 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_deprecated_bad.out b/test_regress/t/t_flag_deprecated_bad.out index c973d5d01..1bedfc2f8 100644 --- a/test_regress/t/t_flag_deprecated_bad.out +++ b/test_regress/t/t_flag_deprecated_bad.out @@ -9,5 +9,6 @@ %Warning-DEPRECATED: Option '-fno-dfg-post-inline' is deprecated and has no effect %Warning-DEPRECATED: Option '-fno-dfg-scoped' is deprecated, use '-fno-dfg' instead. %Warning-DEPRECATED: Option '-fno-dfg-break-cycles' is deprecated and has no effect +%Warning-DEPRECATED: Option '-fno-lift-expr' is deprecated and has no effect %Warning-DEPRECATED: Option '--assert-unroll-limit' is deprecated and has no effect. %Error: Exiting due to diff --git a/test_regress/t/t_flag_deprecated_bad.py b/test_regress/t/t_flag_deprecated_bad.py index d8a4baec2..f70d9a99a 100755 --- a/test_regress/t/t_flag_deprecated_bad.py +++ b/test_regress/t/t_flag_deprecated_bad.py @@ -12,7 +12,7 @@ import vltest_bootstrap test.scenarios('vlt') test.lint(verilator_flags2=[ - "--trace-fst-thread --trace-threads 2 --order-clock-delay --clk foo --no-clk bar -fno-dfg-pre-inline -fno-dfg-post-inline -fno-dfg-scoped -fno-dfg-break-cycles --assert-unroll-limit 1024", + "--trace-fst-thread --trace-threads 2 --order-clock-delay --clk foo --no-clk bar -fno-dfg-pre-inline -fno-dfg-post-inline -fno-dfg-scoped -fno-dfg-break-cycles -fno-lift-expr --assert-unroll-limit 1024", ], fails=True, expect_filename=test.golden_filename) diff --git a/test_regress/t/t_opt_lift_expr.cpp b/test_regress/t/t_opt_lift_expr.cpp new file mode 100644 index 000000000..20e7905b7 --- /dev/null +++ b/test_regress/t/t_opt_lift_expr.cpp @@ -0,0 +1,17 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// +// 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 +// +//************************************************************************* + +#include "Vt_opt_lift_expr__Dpi.h" +#include "svdpi.h" + +svLogic impure_0() { return sv_0; } + +svLogic impure_1() { return sv_1; } diff --git a/test_regress/t/t_opt_lift_expr.py b/test_regress/t/t_opt_lift_expr.py index 2351d6963..c88806d11 100755 --- a/test_regress/t/t_opt_lift_expr.py +++ b/test_regress/t/t_opt_lift_expr.py @@ -11,8 +11,20 @@ import vltest_bootstrap test.scenarios('vlt') -test.compile() +# -fno-const-before-dfg keeps V3Const from folding AstLogIf into AstLogOr, +# which is the only way V3LiftExpr ever encounters an AstLogIf. Option +# -fno-const-before-dfg itself is needed for testing Dfg, so needs to work. +test.compile(verilator_flags2=['--binary', '--stats', '-fno-const-before-dfg', test.pli_filename]) test.execute() +test.file_grep(test.stats, r'LiftExpr, lifted LogAnd\s+(\d+)', 16) +test.file_grep(test.stats, r'LiftExpr, lifted LogOr\s+(\d+)', 16) +test.file_grep(test.stats, r'LiftExpr, lifted LogIf\s+(\d+)', 16) +test.file_grep(test.stats, r'LiftExpr, lifted Cond\s+(\d+)', 32) +test.file_grep(test.stats, r'LiftExpr, lifted calls\s+(\d+)', 178) +test.file_grep(test.stats, r'LiftExpr, lifted impure expressions\s+(\d+)', 2) +test.file_grep(test.stats, r'LiftExpr, temporaries created\s+(\d+)', 196) +test.file_grep(test.stats, r'LiftExpr, temporaries reused\s+(\d+)', 16) + test.passes() diff --git a/test_regress/t/t_opt_lift_expr.v b/test_regress/t/t_opt_lift_expr.v index 2b7b41b52..5db43ffc4 100644 --- a/test_regress/t/t_opt_lift_expr.v +++ b/test_regress/t/t_opt_lift_expr.v @@ -13,6 +13,9 @@ module t; + import "DPI-C" function logic impure_0(); + import "DPI-C" function logic impure_1(); + function automatic int one(); return 1; endfunction @@ -30,6 +33,73 @@ module t; initial begin `checkh(C::i, 2); `checkh(C::j, 3); + // Expr + `checkh(|$urandom_range(2,1), 1'b1); + // LogAnd + `checkh(impure_0() && 1'b0, 1'b0); + `checkh(impure_0() && 1'b1, 1'b0); + `checkh(impure_1() && 1'b0, 1'b0); + `checkh(impure_1() && 1'b1, 1'b1); + `checkh(1'b0 && impure_0(), 1'b0); + `checkh(1'b0 && impure_1(), 1'b0); + `checkh(1'b1 && impure_0(), 1'b0); + `checkh(1'b1 && impure_1(), 1'b1); + `checkh(impure_0() && impure_0(), 1'b0); + `checkh(impure_0() && impure_1(), 1'b0); + `checkh(impure_1() && impure_0(), 1'b0); + `checkh(impure_1() && impure_1(), 1'b1); + // LogOr + `checkh(impure_0() || 1'b0, 1'b0); + `checkh(impure_0() || 1'b1, 1'b1); + `checkh(impure_1() || 1'b0, 1'b1); + `checkh(impure_1() || 1'b1, 1'b1); + `checkh(1'b0 || impure_0(), 1'b0); + `checkh(1'b0 || impure_1(), 1'b1); + `checkh(1'b1 || impure_0(), 1'b1); + `checkh(1'b1 || impure_1(), 1'b1); + `checkh(impure_0() || impure_0(), 1'b0); + `checkh(impure_0() || impure_1(), 1'b1); + `checkh(impure_1() || impure_0(), 1'b1); + `checkh(impure_1() || impure_1(), 1'b1); + // LogIf + `checkh(impure_0() -> 1'b0, 1'b1); + `checkh(impure_0() -> 1'b1, 1'b1); + `checkh(impure_1() -> 1'b0, 1'b0); + `checkh(impure_1() -> 1'b1, 1'b1); + `checkh(1'b0 -> impure_0(), 1'b1); + `checkh(1'b0 -> impure_1(), 1'b1); + `checkh(1'b1 -> impure_0(), 1'b0); + `checkh(1'b1 -> impure_1(), 1'b1); + `checkh(impure_0() -> impure_0(), 1'b1); + `checkh(impure_0() -> impure_1(), 1'b1); + `checkh(impure_1() -> impure_0(), 1'b0); + `checkh(impure_1() -> impure_1(), 1'b1); + // Cond + `checkh(impure_0() ? 1'b0 : 1'b0, 1'b0); + `checkh(impure_0() ? 1'b0 : 1'b1, 1'b1); + `checkh(impure_0() ? 1'b1 : 1'b0, 1'b0); + `checkh(impure_0() ? 1'b1 : 1'b1, 1'b1); + `checkh(impure_1() ? 1'b0 : 1'b0, 1'b0); + `checkh(impure_1() ? 1'b0 : 1'b1, 1'b0); + `checkh(impure_1() ? 1'b1 : 1'b0, 1'b1); + `checkh(impure_1() ? 1'b1 : 1'b1, 1'b1); + `checkh(impure_0() ? impure_0() : 1'b0, 1'b0); + `checkh(impure_0() ? impure_0() : 1'b1, 1'b1); + `checkh(impure_0() ? impure_1() : 1'b0, 1'b0); + `checkh(impure_0() ? impure_1() : 1'b1, 1'b1); + `checkh(impure_1() ? impure_0() : 1'b0, 1'b0); + `checkh(impure_1() ? impure_0() : 1'b1, 1'b0); + `checkh(impure_1() ? impure_1() : 1'b0, 1'b1); + `checkh(impure_1() ? impure_1() : 1'b1, 1'b1); + `checkh(impure_0() ? 1'b0 : impure_0(), 1'b0); + `checkh(impure_0() ? 1'b0 : impure_1(), 1'b1); + `checkh(impure_0() ? 1'b1 : impure_0(), 1'b0); + `checkh(impure_0() ? 1'b1 : impure_1(), 1'b1); + `checkh(impure_1() ? 1'b0 : impure_0(), 1'b0); + `checkh(impure_1() ? 1'b0 : impure_1(), 1'b0); + `checkh(impure_1() ? 1'b1 : impure_0(), 1'b1); + `checkh(impure_1() ? 1'b1 : impure_1(), 1'b1); + // End test $write("*-* All Finished *-*\n"); $finish; end