From 0dc9f779f8a0033f8cbd94889c9ce15ade1ce731 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Tue, 11 Nov 2025 21:46:19 +0000 Subject: [PATCH] Add `-fno-inline-funcs-eager` option to disable excessive inlining (#6682) --- docs/guide/exe_verilator.rst | 2 + src/V3Options.cpp | 1 + src/V3Options.h | 2 + src/V3Task.cpp | 113 +++++++++++++----- test_regress/t/t_func_impure_bad.out | 6 + test_regress/t/t_func_impure_bad.v | 10 ++ test_regress/t/t_opt_inline_funcs_no_eager.py | 19 +++ 7 files changed, 120 insertions(+), 33 deletions(-) create mode 100755 test_regress/t/t_opt_inline_funcs_no_eager.py diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index a2330d902..614320e8e 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -683,6 +683,8 @@ Summary: .. option:: -fno-inline-funcs +.. option:: -fno-inline-funcs-eager + .. option:: -fno-life .. option:: -fno-life-post diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 8d7aeeab3..fac3ebaca 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -1439,6 +1439,7 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, DECL_OPTION("-fgate", FOnOff, &m_fGate); DECL_OPTION("-finline", FOnOff, &m_fInline); DECL_OPTION("-finline-funcs", FOnOff, &m_fInlineFuncs); + DECL_OPTION("-finline-funcs-eager", FOnOff, &m_fInlineFuncsEager); DECL_OPTION("-flife", FOnOff, &m_fLife); DECL_OPTION("-flife-post", FOnOff, &m_fLifePost); DECL_OPTION("-flocalize", FOnOff, &m_fLocalize); diff --git a/src/V3Options.h b/src/V3Options.h index db1506dba..476ea2289 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -405,6 +405,7 @@ private: bool m_fGate; // main switch: -fno-gate: gate wire elimination bool m_fInline; // main switch: -fno-inline: module inlining bool m_fInlineFuncs = true; // main switch: -fno-inline-funcs: function inlining + 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_fLocalize; // main switch: -fno-localize: convert temps to local variables @@ -719,6 +720,7 @@ public: bool fGate() const { return m_fGate; } bool fInline() const { return m_fInline; } bool fInlineFuncs() const { return m_fInlineFuncs; } + bool fInlineFuncsEager() const { return m_fInlineFuncsEager; } bool fLife() const { return m_fLife; } bool fLifePost() const { return m_fLifePost; } bool fLocalize() const { return m_fLocalize; } diff --git a/src/V3Task.cpp b/src/V3Task.cpp index 22b8a449d..1eb7d7ecf 100644 --- a/src/V3Task.cpp +++ b/src/V3Task.cpp @@ -41,6 +41,7 @@ VL_DEFINE_DEBUG_FUNCTIONS; // Graph subclasses class TaskBaseVertex VL_NOT_FINAL : public V3GraphVertex { + VL_RTTI_IMPL(TaskBaseVertex, V3GraphVertex); AstNode* m_impurep = nullptr; // Node causing impure function w/ outside references bool m_noInline = false; // Marked with pragma public: @@ -55,6 +56,7 @@ public: }; class TaskFTaskVertex final : public TaskBaseVertex { + VL_RTTI_IMPL(TaskFTaskVertex, TaskBaseVertex); // Every task gets a vertex, and we link tasks together based on funcrefs. AstNodeFTask* const m_nodep; AstCFunc* m_cFuncp = nullptr; @@ -72,6 +74,7 @@ public: }; class TaskCodeVertex final : public TaskBaseVertex { + VL_RTTI_IMPL(TaskCodeVertex, TaskBaseVertex); // Top vertex for all calls not under another task public: explicit TaskCodeVertex(V3Graph* graphp) @@ -133,38 +136,71 @@ public: void remapFuncClassp(AstNodeFTask* nodep, AstNodeFTask* newp) { m_funcToClassMap[newp] = getClassp(nodep); } - bool ftaskNoInline(AstNodeFTask* nodep) { - return !v3Global.opt.fInlineFuncs() || getFTaskVertex(nodep)->noInline(); - } + bool ftaskNoInline(AstNodeFTask* nodep) { return getFTaskVertex(nodep)->noInline(); } AstCFunc* ftaskCFuncp(AstNodeFTask* nodep) { return getFTaskVertex(nodep)->cFuncp(); } void ftaskCFuncp(AstNodeFTask* nodep, AstCFunc* cfuncp) { getFTaskVertex(nodep)->cFuncp(cfuncp); } - void checkPurity(AstNodeFTask* nodep) { checkPurity(nodep, getFTaskVertex(nodep)); } + // Returns pointer to node that makes the given FTask impure + AstNode* checkImpure(AstNodeFTask* nodep) { return getFTaskVertex(nodep)->impureNode(); } private: - void checkPurity(AstNodeFTask* nodep, TaskBaseVertex* vxp) { - if (nodep->recursive()) return; // Impure, but no warning - if (!vxp->pure()) { - nodep->v3warn( - IMPURE, "Unsupported: External variable referenced by non-inlined function/task: " - << nodep->prettyNameQ() << '\n' - << nodep->warnContextPrimary() << '\n' - << vxp->impureNode()->warnOther() - << "... Location of the external reference: " - << vxp->impureNode()->prettyNameQ() << '\n' - << vxp->impureNode()->warnContextSecondary()); - } - // And, we need to check all tasks this task calls - for (V3GraphEdge& edge : vxp->outEdges()) { - checkPurity(nodep, static_cast(edge.top())); - } - } TaskFTaskVertex* getFTaskVertex(AstNodeFTask* nodep) { if (!nodep->user4p()) nodep->user4p(new TaskFTaskVertex{&m_callGraph, nodep}); return static_cast(nodep->user4u().toGraphVertex()); } + void propagateImpureReason() { + std::vector workList; + // Gather all impure FTask vertices + for (V3GraphVertex& vtx : m_callGraph.vertices()) { + if (TaskFTaskVertex* const ftVtxp = vtx.cast()) { + if (!ftVtxp->pure()) workList.emplace_back(ftVtxp); + } + } + // Mark every pure caller as impure and enqueue them - this propagates impureness + while (!workList.empty()) { + TaskFTaskVertex* const calleep = workList.back(); + workList.pop_back(); + UASSERT_OBJ(!calleep->pure(), calleep->nodep(), "Should not enqueue pure vertex"); + for (V3GraphEdge& edge : calleep->inEdges()) { + if (TaskFTaskVertex* const callerp = edge.fromp()->cast()) { + if (!callerp->pure()) continue; // Already processed or on workList + callerp->impure(calleep->impureNode()); + workList.emplace_back(callerp); + } + } + } + } + + void decideInlining() { + for (V3GraphVertex& vtx : m_callGraph.vertices()) { + TaskFTaskVertex* const ftVtxp = vtx.cast(); + if (!ftVtxp) continue; + // Move on if already decided not to inline + if (ftVtxp->noInline()) continue; + // If inlining blanked disabled, don't do it. Note this usually + // results in large amounts of breakage so is mostly for testing. + if (!v3Global.opt.fInlineFuncs()) { + ftVtxp->noInline(true); + continue; + } + + // If eagerly inlining (historic - and still default - behaviour) then nothing to do + if (v3Global.opt.fInlineFuncsEager()) continue; + + AstNodeFTask* const ftaskp = ftVtxp->nodep(); + // We are relying on inlining of some internal functions for + // functional corretness. Probably that's not a good thing? + if (!ftaskp->verilogFunction()) continue; + // Non-inlned public functions are a bit broken right now, so keep + // inlining them (internal calls are inlined + keep public version) + if (ftaskp->taskPublic()) continue; + // If the function is pure, it's OK not to inline, so don't do it + if (ftVtxp->pure()) ftVtxp->noInline(true); + } + } + // VISITORS void visit(AstScope* nodep) override { // Each FTask is unique per-scope, so AstNodeFTaskRefs do not need @@ -279,6 +315,8 @@ public: iterate(nodep); // m_callGraph.removeRedundantEdgesSum(&TaskEdge::followAlwaysTrue); + propagateImpureReason(); + decideInlining(); if (dumpGraphLevel()) m_callGraph.dumpDotFilePrefixed("task_call"); } ~TaskStateVisitor() override = default; @@ -1520,7 +1558,7 @@ class TaskVisitor final : public VNVisitor { } UINFO(4, " FTask REF Done."); } - void visit(AstNodeFTask* nodep) override { + void visit(AstNodeFTask* const nodep) override { UINFO(4, " visitFTask " << nodep); VL_RESTORER(m_insStmtp); m_insStmtp = nodep->stmtsp(); // Might be null if no statements, but we won't use it @@ -1543,22 +1581,31 @@ class TaskVisitor final : public VNVisitor { << nodep->prettyNameQ()); } - if (nodep->dpiImport() || nodep->dpiExport() || nodep->taskPublic() - || m_statep->ftaskNoInline(nodep)) { + const bool noInline = m_statep->ftaskNoInline(nodep); + // Warn if not inlining an impure ftask (unless method or recursvie). + // Will likely not schedule correctly. + // TODO: Why not if recursive? It will not work ... + if (noInline && !nodep->classMethod() && !nodep->recursive()) { + if (AstNode* const impurep = m_statep->checkImpure(nodep)) { + nodep->v3warn( + IMPURE, + "Unsupported: External variable referenced by non-inlined function/task: " + << nodep->prettyNameQ() << '\n' + << nodep->warnContextPrimary() << '\n' + << impurep->warnOther() << "... Location of the external reference: " + << impurep->prettyNameQ() << '\n' + << impurep->warnContextSecondary()); + } + } + + if (noInline || nodep->dpiImport() || nodep->dpiExport() || nodep->taskPublic()) { // Clone it first, because we may have later FTaskRef's that still need // the original version. - if (m_statep->ftaskNoInline(nodep) && !nodep->classMethod()) { - m_statep->checkPurity(nodep); - } AstNodeFTask* const clonedFuncp = nodep->cloneTree(false); if (nodep->isConstructor()) m_statep->remapFuncClassp(nodep, clonedFuncp); - - AstCFunc* const cfuncp = makeUserFunc(clonedFuncp, m_statep->ftaskNoInline(nodep)); - if (cfuncp) { + if (AstCFunc* const cfuncp = makeUserFunc(clonedFuncp, noInline)) { nodep->addNextHere(cfuncp); - if (nodep->dpiImport() || m_statep->ftaskNoInline(nodep)) { - m_statep->ftaskCFuncp(nodep, cfuncp); - } + if (nodep->dpiImport() || noInline) m_statep->ftaskCFuncp(nodep, cfuncp); iterateIntoFTask(clonedFuncp); // Do the clone too } } diff --git a/test_regress/t/t_func_impure_bad.out b/test_regress/t/t_func_impure_bad.out index 2174bcf9f..b3e1b1acc 100644 --- a/test_regress/t/t_func_impure_bad.out +++ b/test_regress/t/t_func_impure_bad.out @@ -5,4 +5,10 @@ 13 | sig = '1; | ^~~ ... For error description see https://verilator.org/warn/IMPURE?v=latest +%Error-IMPURE: t/t_func_impure_bad.v:20:9: Unsupported: External variable referenced by non-inlined function/task: 't.baz' + 20 | task baz; + | ^~~ + t/t_func_impure_bad.v:17:7: ... Location of the external reference: 't.sig' + 17 | sig = '1; + | ^~~ %Error: Exiting due to diff --git a/test_regress/t/t_func_impure_bad.v b/test_regress/t/t_func_impure_bad.v index 5081b635b..a042b770a 100644 --- a/test_regress/t/t_func_impure_bad.v +++ b/test_regress/t/t_func_impure_bad.v @@ -13,8 +13,18 @@ module t; sig = '1; endtask + task bar; + sig = '1; + endtask + + task baz; + // verilator no_inline_task + bar(); + endtask + initial begin foo(); + baz(); end endmodule diff --git a/test_regress/t/t_opt_inline_funcs_no_eager.py b/test_regress/t/t_opt_inline_funcs_no_eager.py new file mode 100755 index 000000000..01ee79adf --- /dev/null +++ b/test_regress/t/t_opt_inline_funcs_no_eager.py @@ -0,0 +1,19 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2024 by Wilson Snyder. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('vlt') +test.top_filename = "t/t_opt_inline_funcs.v" + +test.compile(verilator_flags2=['--fno-inline-funcs-eager', '--stats'], verilator_make_gmake=False) + +test.file_grep(test.stats, r'Optimizations, Functions inlined\s+(\d+)', 1) + +test.passes()