From 7523a0cd8e3bb564f3cb2ae4598e6152e1d39f02 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Wed, 19 Aug 2026 11:00:47 +0200 Subject: [PATCH] Fix exponential expansion of nested selects (#8137) (#8151) Fixes #8137 --- src/V3Ast.h | 8 + src/V3Expand.cpp | 193 ++++++++++++------ .../t/t_opt_expand_sel_wide_nested.py | 23 +++ test_regress/t/t_opt_expand_sel_wide_nested.v | 91 +++++++++ 4 files changed, 258 insertions(+), 57 deletions(-) create mode 100755 test_regress/t/t_opt_expand_sel_wide_nested.py create mode 100644 test_regress/t/t_opt_expand_sel_wide_nested.v diff --git a/src/V3Ast.h b/src/V3Ast.h index 5a4e95245..fc3d277e5 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -1261,6 +1261,14 @@ public: this->foreach([&count](const AstNode*) { ++count; }); return count; } + + // Return true if and only if the tree rooted at this node has more than 'limit' nodes. + // Traversal terminates as soon as the result is known, so unlike comparing 'nodeCount', + // this is cheap on a large tree. + bool isLargerThan(int limit) const { + int count = 0; + return this->exists([&count, limit](const AstNode*) { return ++count > limit; }); + } }; // Forward declarations of specializations defined in V3Ast.cpp diff --git a/src/V3Expand.cpp b/src/V3Expand.cpp index f58474273..ec573cff0 100644 --- a/src/V3Expand.cpp +++ b/src/V3Expand.cpp @@ -489,83 +489,162 @@ class ExpandVisitor final : public VNVisitor { void visit(AstSel* nodep) override { if (nodep->user1SetOnce()) return; // Process once iterateChildren(nodep); - // Remember, Sel's may have non-integer rhs, so need to optimize for that! - UASSERT_OBJ(nodep->widthMin() == nodep->widthConst(), nodep, "Width mismatch"); - if (VN_IS(nodep->backp(), NodeAssign) - && nodep == VN_AS(nodep->backp(), NodeAssign)->lhsp()) { - // Sel is an LHS assignment select - } else if (nodep->isWide()) { - // See under ASSIGN(WIDE) - } else if (VN_IS(nodep->fromp()->dtypep(), StreamDType) - || VN_IS(nodep->fromp()->dtypep(), QueueDType)) { - //sel stream or queue - } else if (nodep->fromp()->isWide()) { + + const uint32_t width = static_cast(nodep->widthConst()); + UASSERT_OBJ(nodep->widthMin() == static_cast(width), nodep, "Width mismatch"); + + // Skip if Sel is an LHS assignment select + if (AstNodeAssign* const assignp = VN_CAST(nodep->backp(), NodeAssign)) { + if (nodep == assignp->lhsp()) return; + } + + // Skip if wide: See under ASSIGN(WIDE) + if (nodep->isWide()) return; + + // Skip Sel from stream or queue + if (VN_IS(nodep->fromp()->dtypep(), StreamDType)) return; + if (VN_IS(nodep->fromp()->dtypep(), QueueDType)) return; + + // Result must be non-wide after skipping all the above + UASSERT_OBJ(width <= 64, nodep, "Inconsistent result width"); + + if (nodep->fromp()->isWide()) { // Long/Quad from Wide if (isImpure(nodep)) return; UINFO(8, " SEL(wide) " << nodep); - UASSERT_OBJ(nodep->widthConst() <= 64, nodep, "Inconsistent width"); - // Selection amounts - // Check for constant shifts & save some constification work later. - // Grab lowest bit(s) FileLine* const nfl = nodep->fileline(); FileLine* const lfl = nodep->lsbp()->fileline(); FileLine* const ffl = nodep->fromp()->fileline(); - AstNodeExpr* lowwordp = newWordSelBit(ffl, nodep->fromp(), nodep->lsbp()); - if (nodep->isQuad() && !lowwordp->isQuad()) { - lowwordp = new AstCCast{nfl, lowwordp, nodep}; + AstNodeExpr* const fromp = nodep->fromp(); + + // To extract an up-to-64-bit value from a wide source, up to 3 words might be needed + // from the source operand. Each word might need to be shifted by a different amount. + // All word indices and shift amounts are derived from the LSB expression, so it might + // be duplicated several times. The problem arises when the LSB expression itself is + // a similar select, which results in an exponential size expansion. + // + // To avoid this, if the required expressions derived from the LSB expression are + // large, and the LSB expression is required multiple times for the expansion, then + // the LSB expression is evaluated into a temporary variable. This bounds the size + // of the expansion on each Sel instance, hence the total expansion of nested Sels + // is bounded linearly with the number of nested Sels (instead of exponentially). + + // Return simplified 'exprp' if it simplifies to a small tree, otherwise nullptr + const auto tryFold = [&](AstNodeExpr* exprp) -> AstNodeExpr* { + // Size limit for inlining expressions (any arbitrary value results in + // a bounded expansion. Picked a value to allow common simple forms.) + static constexpr int EXPAND_SEL_LSB_LIMIT = 8; + // Simplify + exprp = V3Const::constifyEditCpp(exprp); + // Accept if not larger than the limit + if (!exprp->isLargerThan(EXPAND_SEL_LSB_LIMIT)) return exprp; + // Delete if rejected + VL_DO_DANGLING(exprp->deleteTree(), exprp); + return nullptr; + }; + + // Return the index of the word holding the bit 'offset' bits above the LSB + const auto wordIdx = [&](uint32_t offset) -> AstNodeExpr* { + AstNodeExpr* const msbp = new AstAdd{lfl, new AstConst{lfl, offset}, + nodep->lsbp()->cloneTreePure(false)}; + AstNodeExpr* const idxp = newWordIndex(msbp); + if (!msbp->backp()) VL_DO_DANGLING(msbp->deleteTree(), msbp); + return idxp; + }; + + // Bit index of the selected LSB within its word + AstNodeExpr* lBitp = tryFold(newSelBitBit(nodep->lsbp())); + const bool aligned = lBitp && lBitp->isZero(); + + // Which of the other 2 words the expansion might need + const bool mNeeded = width > 1 && !aligned; + const bool hNeeded = width > VL_EDATASIZE; + UASSERT_OBJ(!hNeeded || nodep->isQuad(), nodep, "Width mismatch"); + + // Word indices + const uint32_t mMsbOffset = std::min(width, VL_EDATASIZE) - 1; + const uint32_t hMsbOffset = width - 1; + AstNodeExpr* lIdxp = tryFold(newWordIndex(nodep->lsbp())); + AstNodeExpr* mIdxp = mNeeded ? tryFold(wordIdx(mMsbOffset)) : nullptr; + AstNodeExpr* hIdxp = hNeeded ? tryFold(wordIdx(hMsbOffset)) : nullptr; + + // If the LSB expression is needed multiple times, evaluate it into a temporary + const int nCopies = !lBitp + !lIdxp + (mNeeded && !mIdxp) + (hNeeded && !hIdxp); + if (nCopies > 1 && m_funcp && m_stmtp && !isImpure(m_stmtp)) { + ++m_nTmps; // Use fresh set of temporaries + AstNodeExpr* const lsbp = nodep->lsbp()->unlinkFrBack(); + AstVar* const tmpp = addLocalTmp(m_stmtp, "ExpandSel_Lsb", lsbp); + nodep->lsbp(new AstVarRef{lfl, tmpp, VAccess::READ}); } - AstNodeExpr* const lowp - = new AstShiftR{nfl, lowwordp, newSelBitBit(nodep->lsbp()), nodep->width()}; - // If > 1 bit, we might be crossing the word boundary - AstNodeExpr* midp = nullptr; - if (nodep->widthConst() > 1) { - const uint32_t midMsbOffset - = std::min(nodep->widthConst(), VL_EDATASIZE) - 1; - AstNodeExpr* const midMsbp = new AstAdd{lfl, new AstConst{lfl, midMsbOffset}, - nodep->lsbp()->cloneTreePure(true)}; - AstNodeExpr* midwordp = newWordSelBit(ffl, nodep->fromp(), midMsbp, 0); - if (!midMsbp->backp()) VL_DO_DANGLING(midMsbp->deleteTree(), midMsbp); - if (nodep->isQuad() && !midwordp->isQuad()) { - midwordp = new AstCCast{nfl, midwordp, nodep}; - } - AstNodeExpr* const midshiftp = new AstSub{lfl, new AstConst{lfl, VL_EDATASIZE}, - newSelBitBit(nodep->lsbp())}; + + // Rebuild remaining expressions derived from LSB if needed, now using the temporary + if (!lBitp) lBitp = newSelBitBit(nodep->lsbp()); + if (!lIdxp) lIdxp = newWordIndex(nodep->lsbp()); + if (!mIdxp) mIdxp = mNeeded ? wordIdx(mMsbOffset) : nullptr; + if (!hIdxp) hIdxp = hNeeded ? wordIdx(hMsbOffset) : nullptr; + + // Return word 'idxp' of 'fromp', without consuming 'idxp' + const auto wordSel = [&](AstNodeExpr* idxp) -> AstNodeExpr* { + AstNodeExpr* const clonep = idxp->cloneTreePure(false); + AstNodeExpr* const wordp = newWordSelWord(ffl, fromp, clonep); + if (!clonep->backp()) VL_DO_DANGLING(clonep->deleteTree(), clonep); + return wordp; + }; + + // Construct term containing the bits of the low word - always needed + AstNodeExpr* const lTermp = [&]() -> AstNodeExpr* { + AstNodeExpr* lWordp = wordSel(lIdxp); + if (nodep->isQuad()) lWordp = new AstCCast{nfl, lWordp, nodep}; + return new AstShiftR{nfl, lWordp, lBitp->cloneTreePure(false), nodep->width()}; + }(); + + // Construct term containing the bits of the middle word - if needed + AstNodeExpr* const mTermp = [&]() -> AstNodeExpr* { + if (!mNeeded) return nullptr; + + AstNodeExpr* mWordp = wordSel(mIdxp); + if (nodep->isQuad()) mWordp = new AstCCast{nfl, mWordp, nodep}; + AstNodeExpr* const mShiftp = new AstSub{lfl, new AstConst{lfl, VL_EDATASIZE}, + lBitp->cloneTreePure(false)}; // If we're selecting bit zero, then all 32 bits in the mid word // get shifted << by 32 bits, so ignore them. const V3Number zero{nodep, longOrQuadWidth(nodep)}; - midp = new AstCond{ + return new AstCond{ nfl, // lsb % VL_EDATASIZE == 0 ? - new AstEq{nfl, new AstConst{nfl, 0}, newSelBitBit(nodep->lsbp())}, + new AstEq{nfl, new AstConst{nfl, 0}, lBitp->cloneTreePure(false)}, // 0 : new AstConst{nfl, zero}, // midword >> (VL_EDATASIZE - (lbs % VL_EDATASIZE)) - new AstShiftL{nfl, midwordp, midshiftp, nodep->width()}}; - } - // If > 32 bits, we might be crossing the second word boundary - AstNodeExpr* hip = nullptr; - if (nodep->widthConst() > VL_EDATASIZE) { - const uint32_t hiMsbOffset = nodep->widthConst() - 1; - AstNodeExpr* const hiMsbp = new AstAdd{lfl, new AstConst{lfl, hiMsbOffset}, - nodep->lsbp()->cloneTreePure(true)}; - AstNodeExpr* hiwordp = newWordSelBit(ffl, nodep->fromp(), hiMsbp); - if (!hiMsbp->backp()) VL_DO_DANGLING(hiMsbp->deleteTree(), hiMsbp); - if (nodep->isQuad() && !hiwordp->isQuad()) { - hiwordp = new AstCCast{nfl, hiwordp, nodep}; - } - AstNodeExpr* const hishiftp = new AstCond{ + new AstShiftL{nfl, mWordp, mShiftp, nodep->width()}}; + }(); + + // Construct term containing the bits of the high word - if needed + AstNodeExpr* const hTermp = [&]() -> AstNodeExpr* { + if (!hNeeded) return nullptr; + + AstNodeExpr* hWordp = wordSel(hIdxp); + hWordp = new AstCCast{nfl, hWordp, nodep}; + AstNodeExpr* const hShiftp = new AstCond{ nfl, // lsb % VL_EDATASIZE == 0 ? - new AstEq{nfl, new AstConst{nfl, 0}, newSelBitBit(nodep->lsbp())}, + new AstEq{nfl, new AstConst{nfl, 0}, lBitp->cloneTreePure(false)}, // VL_EDATASIZE : new AstConst{lfl, VL_EDATASIZE}, // 64 - (lbs % VL_EDATASIZE) - new AstSub{lfl, new AstConst{lfl, 64}, newSelBitBit(nodep->lsbp())}}; - hip = new AstShiftL{nfl, hiwordp, hishiftp, nodep->width()}; - } + new AstSub{lfl, new AstConst{lfl, 64}, lBitp->cloneTreePure(false)}}; + return new AstShiftL{nfl, hWordp, hShiftp, nodep->width()}; + }(); - AstNodeExpr* newp = lowp; - if (midp) newp = new AstOr{nfl, midp, newp}; - if (hip) newp = new AstOr{nfl, hip, newp}; + // Delete parts not captured during construction + VL_DO_DANGLING(lBitp->deleteTree(), lBitp); + VL_DO_DANGLING(lIdxp->deleteTree(), lIdxp); + if (mIdxp) VL_DO_DANGLING(mIdxp->deleteTree(), mIdxp); + if (hIdxp) VL_DO_DANGLING(hIdxp->deleteTree(), hIdxp); + + // Or reduce the terms + AstNodeExpr* newp = lTermp; + if (mTermp) newp = new AstOr{nfl, mTermp, newp}; + if (hTermp) newp = new AstOr{nfl, hTermp, newp}; newp->dtypeFrom(nodep); VL_DO_DANGLING(replaceWithDelete(nodep, newp), nodep); } else { // Long/Quad from Long/Quad diff --git a/test_regress/t/t_opt_expand_sel_wide_nested.py b/test_regress/t/t_opt_expand_sel_wide_nested.py new file mode 100755 index 000000000..e5357cdbf --- /dev/null +++ b/test_regress/t/t_opt_expand_sel_wide_nested.py @@ -0,0 +1,23 @@ +#!/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(verilator_flags2=["--binary", "--stats"]) + +memUsageMB = int(test.file_grep(test.stats, r'Peak Memory Usage \(MB\) +(\d+)')[0]) + +if memUsageMB > 128 and not test.have_dev_asan: + test.error("Consumed over 128MB memory") + +test.execute() + +test.passes() diff --git a/test_regress/t/t_opt_expand_sel_wide_nested.v b/test_regress/t/t_opt_expand_sel_wide_nested.v new file mode 100644 index 000000000..ff0de62fa --- /dev/null +++ b/test_regress/t/t_opt_expand_sel_wide_nested.v @@ -0,0 +1,91 @@ +// 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 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; + + //-------------------------------------------------------------------- + // Stimulus/test driver + + logic clk = 0; + always #5 clk = ~clk; + int cyc = 0; + logic [31:0] rng = 32'h1234_5678; + + function automatic logic [31:0] xorshift(input logic [31:0] x); + logic [31:0] r; + r = x ^ (x << 13); + r = r ^ (r >> 17); + r = r ^ (r << 5); + return r; + endfunction + + always @(posedge clk) begin + cyc <= cyc + 1; + rng <= xorshift(rng); + if (cyc == 500) begin + $write("*-* All Finished *-*\n"); + $finish; + end + end + + //-------------------------------------------------------------------- + // Deeply nested variable offset selects from a wide value. + + logic [15:0][31:0] lut = '0; + logic [31:0] arr[16] = '{default: 32'h0}; // Same as 'lut', but unpacked + logic [3:0] start = 4'h0; + + // Explicitly nested so test doesn't depend on unrolling/dfg, or other opts + wire [3:0] chainLut + = 4'(lut[4'(lut[4'(lut[4'(lut[4'(lut[4'(lut[4'(lut[4'(lut[start])])])])])])])]); + wire [3:0] chainArr + = 4'(arr[4'(arr[4'(arr[4'(arr[4'(arr[4'(arr[4'(arr[4'(arr[start])])])])])])])]); + + // Check exponential expansion + always @(posedge clk) begin + `checkh(chainLut, chainArr); + + // Shift a new entry into 'lut' and into its reference array in step + lut <= {lut[14:0], rng}; + for (int i = 15; i > 0; --i) arr[i] <= arr[i-1]; + arr[0] <= rng; + start <= rng[3:0]; + end + + //-------------------------------------------------------------------- + // Check access boundaries + + logic [511:0] data = 512'h0; + logic [8:0] lsb = 9'h0; + + wire [0:0] sel01 = data[lsb+:01]; + wire [3:0] sel04 = data[lsb+:04]; + wire [30:0] sel31 = data[lsb+:31]; + wire [31:0] sel32 = data[lsb+:32]; + wire [32:0] sel33 = data[lsb+:33]; + wire [63:0] sel64 = data[lsb+:64]; + + always @(posedge clk) begin + `checkh(sel01, 1'(data >> lsb)); + `checkh(sel04, 4'(data >> lsb)); + `checkh(sel31, 31'(data >> lsb)); + `checkh(sel32, 32'(data >> lsb)); + `checkh(sel33, 33'(data >> lsb)); + `checkh(sel64, 64'(data >> lsb)); + + // Sweep every offset, so all word alignments are covered. Stop at 448, + // so that even the widest select stays in range and the reference is + // defined. + lsb <= 9'(cyc % 449); + data <= {data[479:0], ~rng}; + end + +endmodule