From 6eced23e841fa2880f1ff5cada1f11aeb9788173 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Tue, 22 Sep 2026 21:27:21 +0200 Subject: [PATCH] Optimize instance port connections by aliasing rather than by assignment (#8427) Use AstAlias instead of AstAssignW in V3Inst port connection lowering, the same way as V3Inline would do if the instance is inlined. This closes some of the gap between -finline and -fno-inline behaviour, and is also required for moving V3Inline post scope, where it could not create aliases anymore (hence required for future patch to maintain current behaviour). This is also a partial fix to #4698 (only when the connected expression is a plain VarRef) --- src/V3DfgOptimizer.cpp | 7 -- src/V3FsmDetect.cpp | 7 +- src/V3Inst.cpp | 90 +++++++++++++++++++---- src/V3LinkDot.cpp | 15 +++- src/V3Width.cpp | 1 + test_regress/t/t_var_ref_port.py | 18 +++++ test_regress/t/t_var_ref_port.v | 68 +++++++++++++++++ test_regress/t/t_var_ref_port_noinline.py | 20 +++++ 8 files changed, 200 insertions(+), 26 deletions(-) create mode 100755 test_regress/t/t_var_ref_port.py create mode 100644 test_regress/t/t_var_ref_port.v create mode 100755 test_regress/t/t_var_ref_port_noinline.py diff --git a/src/V3DfgOptimizer.cpp b/src/V3DfgOptimizer.cpp index 7abfd6788..e577a0997 100644 --- a/src/V3DfgOptimizer.cpp +++ b/src/V3DfgOptimizer.cpp @@ -183,13 +183,6 @@ class DataflowOptimize final { } DataflowOptimize(AstNetlist* netlistp) { - // Mark interfaces that might be referenced by a virtual interface - if (v3Global.hasVirtIfaces()) { - netlistp->typeTablep()->foreach([](const AstIfaceRefDType* nodep) { - if (!nodep->isVirtual()) return; - nodep->ifaceViaCellp()->setHasVirtualRef(); - }); - } // Mark variables with external references markExternallyReferencedVariables(netlistp); // Dump stage stats diff --git a/src/V3FsmDetect.cpp b/src/V3FsmDetect.cpp index ae92f0459..ef7c64162 100644 --- a/src/V3FsmDetect.cpp +++ b/src/V3FsmDetect.cpp @@ -599,8 +599,9 @@ class FsmDetectVisitor final : public VNVisitor { if (!fsmRegisterWrapperDesc(cellp)) return; UASSERT_OBJ(lhsVscp->varp()->isInput(), nodep, "Child-side port alias lhs should be an input"); - UASSERT_OBJ(rhsVscp->scopep() == m_scopep, nodep, - "Child input port alias should connect from the parent scope"); + // Note the connected variable can live in a scope further up, as V3Inst + // aliases a port connection, merging the port variable of an instance into + // whatever it is connected to m_cellPortAliases[cellp][lhsVscp->varp()->name()] = rhsVscp; m_cellPortChildAliases[cellp][lhsVscp->varp()->name()] = lhsVscp; addWrapperCell(m_scopep, cellp); @@ -608,8 +609,6 @@ class FsmDetectVisitor final : public VNVisitor { if (!fsmRegisterWrapperDesc(cellp)) return; UASSERT_OBJ(rhsVscp->varp()->isWritable(), nodep, "Child-side port alias rhs should be writable"); - UASSERT_OBJ(lhsVscp->scopep() == m_scopep, nodep, - "Child output port alias should connect into the parent scope"); m_cellPortAliases[cellp][rhsVscp->varp()->name()] = lhsVscp; m_cellPortChildAliases[cellp][rhsVscp->varp()->name()] = rhsVscp; addWrapperCell(m_scopep, cellp); diff --git a/src/V3Inst.cpp b/src/V3Inst.cpp index 9ed0eeab9..b6ce2727e 100644 --- a/src/V3Inst.cpp +++ b/src/V3Inst.cpp @@ -26,6 +26,7 @@ #include "V3Inst.h" #include "V3Const.h" +#include "V3Control.h" #include "V3Width.h" VL_DEFINE_DEBUG_FUNCTIONS; @@ -48,6 +49,56 @@ class InstVisitor final : public VNVisitor { // STATE AstCell* m_cellp = nullptr; // Current cell + // METHODS + // If appropriate, add an AstAlias to connect the given Cell pin to the given expression. + // Returns true if an alias was made, in which case there is nothing else to do for this pin. + bool tryAliasPin(AstPin* nodep, AstNodeExpr* exprp) { + AstVar* const modVarp = nodep->modVarp(); + // An interface reference is aliased via AstAliasScope below, not as a variable + if (modVarp->isIfaceRef()) return false; + // Only a whole variable can be aliased, anything else needs the assignment + AstVarRef* const refp = VN_CAST(exprp, VarRef); + if (!refp) return false; + AstVar* const exprVarp = refp->varp(); + // A ref port must become an alias + if (!modVarp->direction().isRef()) { + // V3FsmDetect recognizes the registers of an fsm_register_wrapper instance via + // the assignments of its pins, so leave those as assignments + AstNodeModule* const cellModp = m_cellp->modp(); + if (V3Control::getFsmRegisterWrapper(cellModp->origName()) + || V3Control::getFsmRegisterWrapper(cellModp->prettyDehashOrigOrName())) { + return false; + } + // A virtual interface method call is dispatched at run time, so the body of an + // interface reached that way must use the signals of the instance it is called + // on, not those of whichever instance this connection happens to be made to + if (const AstIface* const ifacep = VN_CAST(m_cellp->modp(), Iface)) { + if (ifacep->hasVirtualRef()) return false; + } + // Forced signals must keep their own storage, the two sides can be forced separately + if (modVarp->isForced() || exprVarp->isForced()) return false; + // Same for public + if (modVarp->isSigUserRWPublic() || exprVarp->isSigUserRWPublic()) return false; + // V3Tristate resolved the connected net already, and drives it from the + // resolution it built for it, so a port merged into it would be driven by + // the resolution of the instance as well + if (exprVarp->isTristate()) return false; + } + // They will become the same variable, so propagate file-line and attributes + exprVarp->fileline()->modifyStateInherit(modVarp->fileline()); + modVarp->fileline()->modifyStateInherit(exprVarp->fileline()); + exprVarp->propagateAttrFrom(modVarp); + modVarp->propagateAttrFrom(exprVarp); + // The port is named first, so the net it connects to is the one that survives + refp->access(VAccess::READWRITE); + FileLine* const flp = exprp->fileline(); + AstNodeExpr* const itemsp + = new AstVarXRef{flp, modVarp, m_cellp->name(), VAccess::READWRITE}; + itemsp->addNext(exprp); + m_cellp->addNextHere(new AstAlias{flp, itemsp}); + return true; + } + // VISITORS void visit(AstCell* nodep) override { UINFO(4, " CELL " << nodep); @@ -57,6 +108,7 @@ class InstVisitor final : public VNVisitor { AstNode::user1ClearTree(); iterateChildren(nodep); } + void visit(AstPin* nodep) override { // PIN(p,expr) -> ASSIGNW(VARXREF(p),expr) (if sub's input) // or ASSIGNW(expr,VARXREF(p)) (if sub's output) @@ -78,21 +130,25 @@ class InstVisitor final : public VNVisitor { if (nodep->modVarp()->isInout()) { nodep->v3fatalSrc("Unsupported: Verilator is a 2-state simulator"); } else if (nodep->modVarp()->isWritable()) { - AstNodeExpr* const rhsp = new AstVarXRef{exprp->fileline(), nodep->modVarp(), - m_cellp->name(), VAccess::READ}; - markContinuousLhs(exprp); - AstAssignW* const assp = new AstAssignW{exprp->fileline(), exprp, rhsp}; - m_cellp->addNextHere(new AstAlways{assp}); + if (!tryAliasPin(nodep, exprp)) { + AstNodeExpr* const rhsp = new AstVarXRef{exprp->fileline(), nodep->modVarp(), + m_cellp->name(), VAccess::READ}; + markContinuousLhs(exprp); + AstAssignW* const assp = new AstAssignW{exprp->fileline(), exprp, rhsp}; + m_cellp->addNextHere(new AstAlways{assp}); + } } else if (nodep->modVarp()->isNonOutput()) { - // Don't bother moving constants now, - // we'll be pushing the const down to the cell soon enough. - AstVarXRef* const lhsp = new AstVarXRef{exprp->fileline(), nodep->modVarp(), - m_cellp->name(), VAccess::WRITE}; + if (!tryAliasPin(nodep, exprp)) { + // Don't bother moving constants now, + // we'll be pushing the const down to the cell soon enough. + AstVarXRef* const lhsp = new AstVarXRef{exprp->fileline(), nodep->modVarp(), + m_cellp->name(), VAccess::WRITE}; - markContinuousLhs(lhsp); - AstAssignW* const assp = new AstAssignW{exprp->fileline(), lhsp, exprp}; - m_cellp->addNextHere(new AstAlways{assp}); - UINFOTREE(9, assp, "", "_new"); + markContinuousLhs(lhsp); + AstAssignW* const assp = new AstAssignW{exprp->fileline(), lhsp, exprp}; + m_cellp->addNextHere(new AstAlways{assp}); + UINFOTREE(9, assp, "", "_new"); + } } else if (nodep->modVarp()->isIfaceRef() || (VN_IS(nodep->modVarp()->dtypep()->skipRefp(), UnpackArrayDType) && VN_IS(VN_AS(nodep->modVarp()->dtypep()->skipRefp(), UnpackArrayDType) @@ -127,7 +183,13 @@ class InstVisitor final : public VNVisitor { public: // CONSTRUCTORS - explicit InstVisitor(AstNetlist* nodep) { iterate(nodep); } + explicit InstVisitor(AstNetlist* nodep) { + // Modules are level sorted, with the top module first. Visit them in reverse + // order, that is children before parents, so that the warning disables and the + // attributes of a port variable propagate all the way up through a chain of + // aliased port connections (see tryAliasPin). + iterateChildrenBackwardsConst(nodep); + } ~InstVisitor() override = default; }; diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 865086b72..bd298d118 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -2759,7 +2759,20 @@ private: UINFOTREE(9, nodep, "", "alias"); AstVarScope* aliasVscp = nullptr; for (AstNode* itemp = nodep->itemsp(); itemp; itemp = itemp->nextp()) { - AstVarScope* const vscp = VN_AS(itemp, VarRef)->varScopep(); + AstVarScope* vscp = nullptr; + if (const AstVarRef* const refp = VN_CAST(itemp, VarRef)) { + vscp = refp->varScopep(); + } else { + // Reaches into the scope of an instance, look it up by name + const AstVarXRef* const xrefp = VN_AS(itemp, VarXRef); + const string scopename = xrefp->dotted() + "." + xrefp->name(); + string baddot; + VSymEnt* okSymp; + VSymEnt* const symp = m_statep->findDotted(xrefp->fileline(), m_modSymp, scopename, + baddot, okSymp, false); + UASSERT_OBJ(symp, nodep, "No symbol for alias item: " << scopename); + vscp = VN_CAST(symp->nodep(), VarScope); + } UASSERT_OBJ(vscp, nodep, "VarScope unset"); if (aliasVscp) { setAliasVarScope(aliasVscp, vscp); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index a4725f26e..4659c85d1 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -3853,6 +3853,7 @@ class WidthVisitor final : public VNVisitor { UINFO(5, " IFACEREF " << nodep); userIterateChildren(nodep, m_vup); nodep->dtypep(nodep); + if (nodep->isVirtual()) nodep->ifaceViaCellp()->setHasVirtualRef(); UINFO(4, "dtWidthed " << nodep); } void visit(AstNodeUOrStructDType* nodep) override { diff --git a/test_regress/t/t_var_ref_port.py b/test_regress/t/t_var_ref_port.py new file mode 100755 index 000000000..46d1fe4c0 --- /dev/null +++ b/test_regress/t/t_var_ref_port.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('simulator') + +test.compile(verilator_flags2=['--binary']) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_var_ref_port.v b/test_regress/t/t_var_ref_port.v new file mode 100644 index 000000000..6bcce625f --- /dev/null +++ b/test_regress/t/t_var_ref_port.v @@ -0,0 +1,68 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// verilog_format: off +`define stop $stop +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +// verilog_format: on + +// A ref port is another name for the variable it is connected to, so a write at +// either end must be seen at the other. +module sub ( + input bit clk, + input int cyc, + ref int y +); + always @(posedge clk) begin + if (cyc == 1) y <= 100; + else if (cyc == 2) `checkd(y, 100) + else if (cyc == 4) `checkd(y, 200) // Written by 't' + end +endmodule + +// A ref port read from a task of the instance. +module subtask ( + ref int z +); + task static check(int expv); + `checkd(z, expv) + endtask +endmodule + +module t; + + bit clk = 0; + always #5 clk = ~clk; + + int cyc = 0; + // Driven both here and in 'sub' via the ref port - that being the point of this test + // verilator lint_off MULTIDRIVEN + int x; + // verilator lint_on MULTIDRIVEN + int w = 15; + + sub s (clk, cyc, x); + subtask st (.z(w)); + + always @(posedge clk) begin + cyc <= cyc + 1; + if (cyc == 2) begin + `checkd(x, 100) // Written by 's' + end else if (cyc == 3) begin + x <= 200; + end else if (cyc == 4) begin + `checkd(x, 200) + st.check(15); + w = 16; + st.check(16); + end else if (cyc == 5) begin + `checkd(w, 16) + $write("*-* All Finished *-*\n"); + $finish; + end + end + +endmodule diff --git a/test_regress/t/t_var_ref_port_noinline.py b/test_regress/t/t_var_ref_port_noinline.py new file mode 100755 index 000000000..a23af37e1 --- /dev/null +++ b/test_regress/t/t_var_ref_port_noinline.py @@ -0,0 +1,20 @@ +#!/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 + +test.top_filename = "t/t_var_ref_port.v" + +import vltest_bootstrap + +test.scenarios('vlt') + +test.compile(verilator_flags2=['--binary', '-fno-inline']) + +test.execute() + +test.passes()