From 1b2b8afdc127615c2d853e455ec50ffd9007639d Mon Sep 17 00:00:00 2001 From: em2machine <92717390+em2machine@users.noreply.github.com> Date: Mon, 9 Mar 2026 22:32:13 -0400 Subject: [PATCH] Fix wrong $bits() for parameterized interface struct typedefs (#7218) (#7219) --- src/V3Param.cpp | 67 ++++- test_regress/t/t_iface_nested_width2.py | 18 ++ test_regress/t/t_iface_nested_width2.v | 242 ++++++++++++++++++ ..._paramgraph_iface_template_nested_stats.py | 4 +- 4 files changed, 321 insertions(+), 10 deletions(-) create mode 100755 test_regress/t/t_iface_nested_width2.py create mode 100644 test_regress/t/t_iface_nested_width2.v diff --git a/src/V3Param.cpp b/src/V3Param.cpp index d84a3c986..3882b8a3d 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -800,7 +800,8 @@ class ParamProcessor final { const string lastCompBase = (braPos == string::npos) ? lastComp : lastComp.substr(0, braPos); if (lastComp != cloneCellp->name() && lastCompBase != cloneCellp->name()) return false; - if (lastDot == string::npos) return true; // No parent portion to verify + // No parent portion to verify - startModp itself must be the expected parent + if (lastDot == string::npos) return startModp == expectModp; const string parentPath = cellPath.substr(0, lastDot); const AstNodeModule* const resolvedp = V3LinkDotIfaceCapture::followCellPath(startModp, parentPath); @@ -869,6 +870,7 @@ class ParamProcessor final { << (correctModp ? correctModp->name() : "") << endl); if (!correctModp || correctModp->dead()) return; + if (correctModp->parameterizedTemplate()) return; bool fixed = false; if (refp->typedefp()) { @@ -1292,8 +1294,10 @@ class ParamProcessor final { if (resolvedp && (VN_IS(resolvedp, StructDType) || VN_IS(resolvedp, UnionDType))) { AstNodeModule* const ownerModp = V3LinkDotIfaceCapture::findOwnerModule(resolvedp); - // Skip if owned by a parameterized template (not yet specialized) - if (ownerModp && ownerModp->parameterizedTemplate()) { + // Skip if owned by a parameterized interface (template or not + // yet cloned). hasGParam() covers interfaces that haven't + // been cloned yet (parameterizedTemplate() not set). + if (ownerModp && VN_IS(ownerModp, Iface) && ownerModp->hasGParam()) { skipWidthForTemplateStruct = true; V3Stats::addStatSum("Param, Template struct width skips", 1); UINFO(9, "SKIP-WIDTH-TEMPLATE: struct=" @@ -1776,6 +1780,28 @@ class ParamProcessor final { } public: + // After an interface cell inside parentModp has been deparameterized + // (rewired from template to clone), retarget REFDTYPEs that still + // reference the old template's types so that $bits(iface_typedef) + // evaluates with the clone's widths. + void retargetIfaceRefs(AstNodeModule* parentModp, const string& cellName) { + AstNodeModule* const correctModp + = V3LinkDotIfaceCapture::followCellPath(parentModp, cellName); + if (!correctModp || correctModp->dead() || correctModp->parameterizedTemplate()) return; + V3LinkDotIfaceCapture::forEach([&](const V3LinkDotIfaceCapture::CapturedEntry& entry) { + if (!entry.refp || entry.cloneCellPath.empty()) return; + if (entry.cellPath != cellName) return; + // Identity check: only retarget REFDTYPEs that actually live + // inside parentModp. Multiple clones share origName so name + // matching alone would let one clone overwrite another's refs. + if (V3LinkDotIfaceCapture::findOwnerModule(entry.refp) != parentModp) return; + if (retargetRefToModule(entry, correctModp)) { + UINFO(9, "retargetIfaceRefs: " << entry.refp << " -> " + << correctModp->prettyNameQ() << endl); + } + }); + } + AstNodeModule* nodeDeparam(AstNode* nodep, AstNodeModule* srcModp, AstNodeModule* modp, const string& someInstanceName) { // Return new or reused de-parameterized module @@ -1895,7 +1921,11 @@ public: // STATE - across all visitors std::vector m_paramClasses; // Parameterized classes std::vector m_dots; // Dot references to process - std::multimap m_workQueueNext; // Modules left to process + // Work queue keyed by (!isIface, level) so interfaces are always processed + // before non-interfaces. This ensures interface clones have their types + // properly widthed before any module that references those types. + using WQKey = std::pair; + std::multimap m_workQueueNext; // Modules left to process // Map from AstNodeModule to set of all AstNodeModules that instantiates it. std::unordered_map> m_parentps; }; @@ -1930,7 +1960,7 @@ class ParamVisitor final : public VNVisitor { void processWorkQ() { UASSERT(!m_iterateModule, "Should not nest"); - std::multimap workQueue; + std::multimap workQueue; m_generateHierName = ""; m_iterateModule = true; @@ -2005,10 +2035,21 @@ class ParamVisitor final : public VNVisitor { if (VN_IS(srcModp, Iface)) { logTemplateLeakRefs(modp, srcModp, "after queued nodeDeparam", cellp); + // After the interface cell is rewired to its clone, + // retarget REFDTYPEs in the parent module that still + // reference the template interface's types. + if (V3LinkDotIfaceCapture::enabled()) { + if (const AstCell* const modCellp = VN_CAST(cellp, Cell)) { + if (newModp != srcModp) { + m_processor.retargetIfaceRefs(modp, modCellp->name()); + } + } + } } // Add the (now potentially specialized) child module to the work queue - workQueue.emplace(newModp->level(), newModp); + workQueue.emplace(ParamState::WQKey{!VN_IS(newModp, Iface), newModp->level()}, + newModp); // Add to the hierarchy registry m_state.m_parentps[newModp].insert(modp); @@ -2194,6 +2235,14 @@ class ParamVisitor final : public VNVisitor { // destructively widthing the template with default (zero) // values. See t_interface_nested_struct_param.v. m_processor.nodeDeparam(cellp, srcModp, m_modp, m_modp->someInstanceName()); + // After the interface cell is rewired to its clone, + // retarget REFDTYPEs in the parent module that still + // reference the template interface's types. This ensures + // $bits(iface_typedef) evaluates correctly when + // widthParamsEdit runs on subsequent lparams. + if (V3LinkDotIfaceCapture::enabled() && cellp->modp() != srcModp) { + m_processor.retargetIfaceRefs(m_modp, cellp->name()); + } } } @@ -2225,7 +2274,8 @@ class ParamVisitor final : public VNVisitor { UINFO(4, " MOD-under-MOD. " << nodep); // Delay until current module is done. // processWorkQ() (which we are returning to) will process nodep later - m_state.m_workQueueNext.emplace(nodep->level(), nodep); + m_state.m_workQueueNext.emplace( + ParamState::WQKey{!VN_IS(nodep, Iface), nodep->level()}, nodep); return; } @@ -2233,7 +2283,8 @@ class ParamVisitor final : public VNVisitor { if (nodep->isTop() // Tops || VN_IS(nodep, Class) // Moved classes || VN_IS(nodep, Package)) { // Likewise haven't done wrapTopPackages yet - m_state.m_workQueueNext.emplace(nodep->level(), nodep); + m_state.m_workQueueNext.emplace( + ParamState::WQKey{!VN_IS(nodep, Iface), nodep->level()}, nodep); processWorkQ(); } } diff --git a/test_regress/t/t_iface_nested_width2.py b/test_regress/t/t_iface_nested_width2.py new file mode 100755 index 000000000..fb3797b25 --- /dev/null +++ b/test_regress/t/t_iface_nested_width2.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Localparam with package function call using interface param +# +# 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_iface_nested_width2.v b/test_regress/t/t_iface_nested_width2.v new file mode 100644 index 000000000..73985c7a1 --- /dev/null +++ b/test_regress/t/t_iface_nested_width2.v @@ -0,0 +1,242 @@ +// DESCRIPTION: Verilator: Localparam with package function call using interface param +// +// 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 checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); +// verilog_format: on + +package utils; + function automatic int f_bits (int val); + return (val <= 1) ? 1 : $clog2(val); + endfunction +endpackage + +package soc; + // Configuration structure + typedef struct packed { + // CB Configuration + int unsigned CBRids; + int unsigned CBPids; + int unsigned CBFnum; + // Coarse parameters + int unsigned Tile; + int unsigned NumBb; + // CCA Parameters + int unsigned NumDd; + // CC Parameters + int unsigned DDNumW; + int unsigned DDNumWT; + int unsigned DDIds; // check this one + } cfg_t; +endpackage + +package cb; + typedef struct packed { + int unsigned Rids; // ring id assigned to port + int unsigned Pids; // packet id + int unsigned Fnum; // flit number + int unsigned XdatSize; // raw packet data size + } cfg_t; +endpackage + +package bba; + typedef logic [11:0] csr_index_t; + typedef logic [1:0] cmd_bnum_t; + typedef logic [3:0] rq_cmd_t; + typedef logic [3:0] rs_cmd_t; + typedef union packed { + rq_cmd_t rq; + rs_cmd_t rs; + } cmd_t; +endpackage: bba + +interface bb_types_if #(parameter soc::cfg_t cfg=0)(); + // + typedef logic [$clog2(cfg.NumDd)-1:0] bb_index_t; + typedef logic [$clog2(cfg.DDIds)-1:0] trans_id_t; + + typedef struct packed { + logic hdr_vld; + logic [1:0] bnum; + logic is_rq; + bba::cmd_t cmd; + bb_index_t cc_index; + trans_id_t trans_id; + } cmd_meta_t; + + typedef union packed { + logic [cfg.DDNumWT*64-1:0] raw; + logic [cfg.DDNumWT-1:0][63:0] raw_a; + } cmd_data_t; + + typedef struct packed { + cmd_meta_t meta; + cmd_data_t d; + } cmd_beat_t; +endinterface + +interface cb_port_vc_if #(parameter cb::cfg_t cfg=0)(); + + typedef logic [$clog2(cfg.Rids)-1:0] rid_t; + typedef logic [$clog2(cfg.Pids)-1:0] pid_t; + typedef logic [$clog2(cfg.Fnum)-1:0] fnum_t; + typedef logic [cfg.XdatSize-1:0] xdat_t; + + typedef struct packed { + xdat_t dat; + fnum_t fnum; + logic first; + logic last; + rid_t dest_rid; + } tx_beat_t; + + tx_beat_t tx_beat; +endinterface + +module simple_mem #( + parameter int p_num_buffered_rq = 16, + parameter int p_storage_size = 65536 + )( + bb_types_if bb_types, + // CB Interface + cb_port_vc_if cb_vc0_io, + cb_port_vc_if cb_vc1_io + ); + + typedef bb_types.cmd_beat_t cmd_beat_t; + + logic flit_ot_vld_d, flit_ot_vld_q; + cmd_beat_t flit_ot_d, flit_ot_q; + + always_comb begin + cb_vc1_io.tx_beat = 'b0; + + if(flit_ot_vld_q) begin + cb_vc1_io.tx_beat.dat = flit_ot_q; + end + end +endmodule + +module xm_top #( + parameter soc::cfg_t soc_cfg=0)( + // Interface to SMM from CB + cb_port_vc_if cb_vc0_io, + cb_port_vc_if cb_vc1_io + ); + + bb_types_if #(soc_cfg) bb_types(); + + simple_mem #() simple_mem( + .bb_types(bb_types), + .cb_vc0_io(cb_vc0_io), + .cb_vc1_io(cb_vc1_io) + ); +endmodule + +module tile_top #( + parameter soc::cfg_t soc_cfg=0 +)( + ); + + bb_types_if #(soc_cfg) bb_types(); + typedef bb_types.cmd_beat_t cmd_beat_t; + + localparam cb::cfg_t cb_cfg = '{ + Rids : soc_cfg.CBRids, + Pids : soc_cfg.CBPids, + Fnum : soc_cfg.CBFnum, + XdatSize:$bits(cmd_beat_t) + }; + + cb_port_vc_if#(cb_cfg) cb_tm_vc0_io(); + cb_port_vc_if#(cb_cfg) cb_tm_vc1_io(); + + xm_top #( + .soc_cfg(soc_cfg) + ) xm_top( + .cb_vc0_io(cb_tm_vc0_io), + .cb_vc1_io(cb_tm_vc1_io) + ); + +endmodule + +module soc_top #(parameter soc::cfg_t soc_cfg=0)( + + ); + + genvar a; + localparam int p_num_tile = soc_cfg.Tile; + + bb_types_if #(soc_cfg) bb_types(); + typedef bb_types.cmd_beat_t cmd_beat_t; + + // CB and RBUS + localparam cb::cfg_t cb_cfg = '{ + Rids : soc_cfg.CBRids, + Pids : soc_cfg.CBPids, + Fnum : soc_cfg.CBFnum, + XdatSize:$bits(cmd_beat_t) + }; + + // cb interfaces (HTM dead ends) + cb_port_vc_if#(cb_cfg) cb_vc0_io(); + cb_port_vc_if#(cb_cfg) cb_vc1_io(); + + // HTM + xm_top #( + .soc_cfg(soc_cfg) + ) xm_top( + .cb_vc0_io(cb_vc0_io), + .cb_vc1_io(cb_vc1_io) + ); + + generate + for(a=0; a