From 6bb57e463035a94fafaefe652e747303f9c548e0 Mon Sep 17 00:00:00 2001 From: Ryszard Rozak Date: Wed, 9 Apr 2025 12:37:52 +0200 Subject: [PATCH] Fix assignments with stream expressions and unpacked arrays (#5915) --- src/V3AstNodeDType.h | 2 +- src/V3AstNodes.cpp | 6 +- src/V3Const.cpp | 35 +++++---- src/V3Width.cpp | 82 ++++++++++++++------- test_regress/t/t_stream_unpack.v | 41 ++++++++++- test_regress/t/t_stream_unpack_lhs.out | 14 ---- test_regress/t/t_stream_unpack_narrower.out | 16 +++- test_regress/t/t_stream_unpack_narrower.v | 3 + 8 files changed, 132 insertions(+), 67 deletions(-) diff --git a/src/V3AstNodeDType.h b/src/V3AstNodeDType.h index 21a16d8cd..36e5f05a2 100644 --- a/src/V3AstNodeDType.h +++ b/src/V3AstNodeDType.h @@ -157,7 +157,7 @@ public: bool generic() const VL_MT_SAFE { return m_generic; } void generic(bool flag) { m_generic = flag; } std::pair dimensions(bool includeBasic); - uint32_t arrayUnpackedElements(); // 1, or total multiplication of all dimensions + uint32_t arrayUnpackedElements() const; // 1, or total multiplication of all dimensions static int uniqueNumInc() { return ++s_uniqueNum; } const char* charIQWN() const { return (isString() ? "N" : isWide() ? "W" : isQuad() ? "Q" : "I"); diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 06760ebf6..9ad85e30a 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -1016,11 +1016,11 @@ AstNodeDType::CTypeRecursed AstNodeDType::cTypeRecurse(bool compound, bool packe return info; } -uint32_t AstNodeDType::arrayUnpackedElements() { +uint32_t AstNodeDType::arrayUnpackedElements() const { uint32_t entries = 1; - for (AstNodeDType* dtypep = this; dtypep;) { + for (const AstNodeDType* dtypep = this; dtypep;) { dtypep = dtypep->skipRefp(); // Skip AstRefDType/AstTypedef, or return same node - if (AstUnpackArrayDType* const adtypep = VN_CAST(dtypep, UnpackArrayDType)) { + if (const AstUnpackArrayDType* const adtypep = VN_CAST(dtypep, UnpackArrayDType)) { entries *= adtypep->elementsConst(); dtypep = adtypep->subDTypep(); } else { diff --git a/src/V3Const.cpp b/src/V3Const.cpp index a79c69e42..9bc4c739e 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -2169,7 +2169,7 @@ class ConstVisitor final : public VNVisitor { // Add a cast if needed. AstStreamR* const streamp = VN_AS(nodep->rhsp(), StreamR)->unlinkFrBack(); AstNodeExpr* srcp = streamp->lhsp()->unlinkFrBack(); - AstNodeDType* const srcDTypep = srcp->dtypep(); + AstNodeDType* const srcDTypep = srcp->dtypep()->skipRefp(); if (VN_IS(srcDTypep, QueueDType) || VN_IS(srcDTypep, DynArrayDType)) { srcp = new AstCvtArrayToPacked{srcp->fileline(), srcp, nodep->dtypep()}; } else if (VN_IS(srcDTypep, UnpackArrayDType)) { @@ -2191,30 +2191,32 @@ class ConstVisitor final : public VNVisitor { // Push the stream operator to the rhs of the assignment statement AstNodeExpr* streamp = nodep->lhsp()->unlinkFrBack(); AstNodeExpr* const dstp = VN_AS(streamp, StreamL)->lhsp()->unlinkFrBack(); + AstNodeDType* const dstDTypep = dstp->dtypep()->skipRefp(); AstNodeExpr* const srcp = nodep->rhsp()->unlinkFrBack(); + AstNodeDType* const srcDTypep = srcp->dtypep()->skipRefp(); const int sWidth = srcp->width(); const int dWidth = dstp->width(); // Connect the rhs to the stream operator and update its width VN_AS(streamp, StreamL)->lhsp(srcp); - if (VN_IS(srcp->dtypep(), DynArrayDType) || VN_IS(srcp->dtypep(), QueueDType) - || VN_IS(srcp->dtypep(), UnpackArrayDType)) { + if (VN_IS(srcDTypep, DynArrayDType) || VN_IS(srcDTypep, QueueDType) + || VN_IS(srcDTypep, UnpackArrayDType)) { streamp->dtypeSetStream(); } else { streamp->dtypeSetLogicUnsized(srcp->width(), srcp->widthMin(), VSigning::UNSIGNED); } - if (VN_IS(dstp->dtypep(), UnpackArrayDType)) { - streamp = new AstCvtPackedToArray{nodep->fileline(), streamp, dstp->dtypep()}; + if (VN_IS(dstDTypep, UnpackArrayDType)) { + streamp = new AstCvtPackedToArray{nodep->fileline(), streamp, dstDTypep}; } else { UASSERT(sWidth >= dWidth, "sWidth >= dWidth should have caused an error earlier"); if (dWidth == 0) { - streamp = new AstCvtPackedToArray{nodep->fileline(), streamp, dstp->dtypep()}; + streamp = new AstCvtPackedToArray{nodep->fileline(), streamp, dstDTypep}; } else if (sWidth >= dWidth) { streamp = new AstSel{streamp->fileline(), streamp, sWidth - dWidth, dWidth}; } } nodep->lhsp(dstp); nodep->rhsp(streamp); - nodep->dtypep(dstp->dtypep()); + nodep->dtypep(dstDTypep); return true; } else if (m_doV && VN_IS(nodep->lhsp(), StreamR)) { // The right stream operator on lhs of assignment statement does @@ -2222,12 +2224,13 @@ class ConstVisitor final : public VNVisitor { // then we select bits from the left-most, not the right-most. AstNodeExpr* const streamp = nodep->lhsp()->unlinkFrBack(); AstNodeExpr* const dstp = VN_AS(streamp, StreamR)->lhsp()->unlinkFrBack(); + AstNodeDType* const dstDTypep = dstp->dtypep()->skipRefp(); AstNodeExpr* srcp = nodep->rhsp()->unlinkFrBack(); const int sWidth = srcp->width(); const int dWidth = dstp->width(); - if (VN_IS(dstp->dtypep(), UnpackArrayDType)) { + if (VN_IS(dstDTypep, UnpackArrayDType)) { const int dstBitWidth - = dWidth * VN_AS(dstp->dtypep(), UnpackArrayDType)->arrayUnpackedElements(); + = dWidth * VN_AS(dstDTypep, UnpackArrayDType)->arrayUnpackedElements(); // Handling the case where rhs is wider than lhs. StreamL does not require this // since the combination of the left streaming operation and the implicit // truncation in VL_ASSIGN_UNPACK automatically selects the left-most bits. @@ -2235,30 +2238,30 @@ class ConstVisitor final : public VNVisitor { srcp = new AstSel{streamp->fileline(), srcp, sWidth - dstBitWidth, dstBitWidth}; } - srcp = new AstCvtPackedToArray{nodep->fileline(), srcp, dstp->dtypep()}; + srcp = new AstCvtPackedToArray{nodep->fileline(), srcp, dstDTypep}; } else { UASSERT(sWidth >= dWidth, "sWidth >= dWidth should have caused an error earlier"); if (dWidth == 0) { - srcp = new AstCvtPackedToArray{nodep->fileline(), srcp, dstp->dtypep()}; + srcp = new AstCvtPackedToArray{nodep->fileline(), srcp, dstDTypep}; } else if (sWidth >= dWidth) { srcp = new AstSel{streamp->fileline(), srcp, sWidth - dWidth, dWidth}; } } nodep->lhsp(dstp); nodep->rhsp(srcp); - nodep->dtypep(dstp->dtypep()); + nodep->dtypep(dstDTypep); VL_DO_DANGLING(pushDeletep(streamp), streamp); // Further reduce, any of the nodes may have more reductions. return true; } else if (m_doV && VN_IS(nodep->rhsp(), StreamL)) { - AstNodeDType* const lhsDtypep = nodep->lhsp()->dtypep(); + AstNodeDType* const lhsDtypep = nodep->lhsp()->dtypep()->skipRefp(); AstStreamL* streamp = VN_AS(nodep->rhsp(), StreamL); AstNodeExpr* const srcp = streamp->lhsp(); - const AstNodeDType* const srcDTypep = srcp->dtypep(); + const AstNodeDType* const srcDTypep = srcp->dtypep()->skipRefp(); if (VN_IS(srcDTypep, QueueDType) || VN_IS(srcDTypep, DynArrayDType) || VN_IS(srcDTypep, UnpackArrayDType)) { - streamp->lhsp(new AstCvtArrayToPacked{srcp->fileline(), srcp->unlinkFrBack(), - nodep->dtypep()}); + streamp->lhsp( + new AstCvtArrayToPacked{srcp->fileline(), srcp->unlinkFrBack(), lhsDtypep}); streamp->dtypeFrom(lhsDtypep); } } else if (m_doV && replaceAssignMultiSel(nodep)) { diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 3904b975e..c9e32b3f5 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -239,6 +239,13 @@ class WidthVisitor final : public VNVisitor { EXTEND_OFF // No extension }; + int widthUnpacked(const AstNodeDType* const dtypep) { + if (const AstUnpackArrayDType* const arrDtypep = VN_CAST(dtypep, UnpackArrayDType)) { + return arrDtypep->subDTypep()->width() * arrDtypep->arrayUnpackedElements(); + } + return dtypep->width(); + } + static void packIfUnpacked(AstNodeExpr* const nodep) { if (AstUnpackArrayDType* const unpackDTypep = VN_CAST(nodep->dtypep(), UnpackArrayDType)) { const int elementsNum = unpackDTypep->arrayUnpackedElements(); @@ -1586,6 +1593,18 @@ class WidthVisitor final : public VNVisitor { userIterateAndNext(nodep->lhsp(), WidthVP{SELF, BOTH}.p()); // Type set in constructor } + void visit(AstCvtPackedToArray* nodep) override { + if (nodep->didWidthAndSet()) return; + // Opaque returns, so arbitrary + userIterateAndNext(nodep->fromp(), WidthVP{SELF, BOTH}.p()); + // Type set in constructor + } + void visit(AstCvtArrayToPacked* nodep) override { + if (nodep->didWidthAndSet()) return; + // Opaque returns, so arbitrary + userIterateAndNext(nodep->fromp(), WidthVP{SELF, BOTH}.p()); + // Type set in constructor + } void visit(AstTimeImport* nodep) override { // LHS is a real number in seconds // Need to round to time units and precision @@ -5180,6 +5199,43 @@ class WidthVisitor final : public VNVisitor { // if (debug()) nodep->dumpTree("- assign: "); AstNodeDType* const lhsDTypep = nodep->lhsp()->dtypep(); // Note we use rhsp for context determined + + // Check width of stream and wrap if needed + if (AstNodeStream* const streamp = VN_CAST(nodep->rhsp(), NodeStream)) { + AstNodeDType* const lhsDTypeSkippedRefp = lhsDTypep->skipRefp(); + const int lwidth = widthUnpacked(lhsDTypeSkippedRefp); + const int rwidth = widthUnpacked(streamp->lhsp()->dtypep()->skipRefp()); + if (lwidth != 0 && lwidth < rwidth) { + nodep->v3widthWarn(lwidth, rwidth, + "Target fixed size variable (" + << lwidth << " bits) is narrower than the stream (" + << rwidth << " bits) (IEEE 1800-2023 11.4.14)"); + } + if (VN_IS(lhsDTypeSkippedRefp, NodeArrayDType)) { + streamp->unlinkFrBack(); + nodep->rhsp(new AstCvtPackedToArray{streamp->fileline(), streamp, + lhsDTypeSkippedRefp}); + } + } + if (const AstNodeStream* const streamp = VN_CAST(nodep->lhsp(), NodeStream)) { + const AstNodeDType* const rhsDTypep = nodep->rhsp()->dtypep()->skipRefp(); + + const int lwidth = widthUnpacked(streamp->lhsp()->dtypep()->skipRefp()); + const int rwidth = widthUnpacked(rhsDTypep); + if (rwidth != 0 && rwidth < lwidth) { + nodep->v3widthWarn(lwidth, rwidth, + "Stream target requires " + << lwidth + << " bits, but source expression only provides " + << rwidth << " bits (IEEE 1800-2023 11.4.14.3)"); + } + if (VN_IS(rhsDTypep, NodeArrayDType)) { + AstNodeExpr* const rhsp = nodep->rhsp()->unlinkFrBack(); + nodep->rhsp( + new AstCvtArrayToPacked{rhsp->fileline(), rhsp, streamp->dtypep()}); + } + } + iterateCheckAssign(nodep, "Assign RHS", nodep->rhsp(), FINAL, lhsDTypep); // if (debug()) nodep->dumpTree("- AssignOut: "); } @@ -5241,32 +5297,6 @@ class WidthVisitor final : public VNVisitor { return; } - // Width check for unpacked array stream assignment - if (const AstNodeStream* streamp = VN_CAST(nodep->rhsp(), NodeStream)) { - if (AstUnpackArrayDType* arr = VN_CAST(streamp->lhsp()->dtypep(), UnpackArrayDType)) { - int lwidth = nodep->lhsp()->width(); - int rwidth = arr->subDTypep()->width() * arr->arrayUnpackedElements(); - if (lwidth != 0 && lwidth < rwidth) { - nodep->v3widthWarn(lwidth, rwidth, - "Target fixed size variable (" - << lwidth << " bits) is narrower than the stream (" - << rwidth << " bits) (IEEE 1800-2023 11.4.14)"); - } - } - } else if (const AstNodeStream* streamp = VN_CAST(nodep->lhsp(), NodeStream)) { - if (AstUnpackArrayDType* arr = VN_CAST(streamp->lhsp()->dtypep(), UnpackArrayDType)) { - int rwidth = nodep->rhsp()->width(); - int lwidth = arr->subDTypep()->width() * arr->arrayUnpackedElements(); - if (rwidth != 0 && rwidth < lwidth) { - nodep->v3widthWarn(lwidth, rwidth, - "Stream target requires " - << lwidth - << " bits, but source expression only provides " - << rwidth << " bits (IEEE 1800-2023 11.4.14.3)"); - } - } - } - if (nodep->hasDType() && nodep->dtypep()->isEvent()) { checkEventAssignment(nodep); v3Global.setAssignsEvents(); diff --git a/test_regress/t/t_stream_unpack.v b/test_regress/t/t_stream_unpack.v index 5dab67a95..895b56a5f 100644 --- a/test_regress/t/t_stream_unpack.v +++ b/test_regress/t/t_stream_unpack.v @@ -15,10 +15,12 @@ typedef enum bit [5:0] { module t (/*AUTOARG*/); initial begin - bit arr[6]; + typedef bit [5:0] bit6_t; + typedef bit bit6_unpacked_t[6]; + bit6_unpacked_t arr; bit [1:0] arr2[3]; - bit [5:0] arr6[1]; - bit [5:0] bit6 = 6'b111000; + bit6_t arr6[1]; + bit6_t bit6 = 6'b111000; bit [5:0] ans; enum_t ans_enum; logic [1:0] a [3] = {1, 0, 3}; @@ -31,27 +33,45 @@ module t (/*AUTOARG*/); { >> bit {arr}} = bit6; `checkp(arr, "'{'h1, 'h1, 'h1, 'h0, 'h0, 'h0} "); + arr = { >> bit {bit6}}; + `checkp(arr, "'{'h1, 'h1, 'h1, 'h0, 'h0, 'h0} "); + ans = { >> bit {arr} }; `checkh(ans, bit6); + { >> bit {ans}} = arr; + `checkh(ans, bit6); + ans_enum = enum_t'({ >> bit {arr} }); `checkh(ans_enum, bit6); { << bit {arr}} = bit6; `checkp(arr, "'{'h0, 'h0, 'h0, 'h1, 'h1, 'h1} "); + arr = { << bit {bit6}}; + `checkp(arr, "'{'h0, 'h0, 'h0, 'h1, 'h1, 'h1} "); + ans = { << bit {arr} }; `checkh(ans, bit6); + { << bit {ans} } = arr; + `checkh(ans, bit6); + ans_enum = enum_t'({ << bit {arr} }); `checkh(ans_enum, bit6); { >> bit[1:0] {arr2}} = bit6; `checkp(arr2, "'{'h3, 'h2, 'h0} "); + arr2 = { >> bit[1:0] {bit6}}; + `checkp(arr2, "'{'h3, 'h2, 'h0} "); + ans = { >> bit[1:0] {arr2} }; `checkh(ans, bit6); + { >> bit[1:0] {ans} } = arr2; + `checkh(ans, bit6); + ans_enum = enum_t'({ >> bit[1:0] {arr2} }); `checkh(ans_enum, bit6); @@ -61,24 +81,39 @@ module t (/*AUTOARG*/); ans = { << bit[1:0] {arr2} }; `checkh(ans, bit6); + { << bit[1:0] {ans} } = arr2; + `checkh(ans, bit6); + ans_enum = enum_t'({ << bit[1:0] {arr2} }); `checkh(ans_enum, bit6); { >> bit [5:0] {arr6} } = bit6; `checkp(arr6, "'{'h38} "); + arr6 = { >> bit [5:0] {bit6}}; + `checkp(arr6, "'{'h38} "); + ans = { >> bit[5:0] {arr6} }; `checkh(ans, bit6); + { >> bit[5:0] {ans} } = arr6; + `checkh(ans, bit6); + ans_enum = enum_t'({ >> bit[5:0] {arr6} }); `checkh(ans_enum, bit6); { << bit [5:0] {arr6} } = bit6; `checkp(arr6, "'{'h38} "); + arr6 = { << bit [5:0] {bit6}}; + `checkp(arr6, "'{'h38} "); + ans = { << bit[5:0] {arr6} }; `checkh(ans, bit6); + { << bit[5:0] {ans} } = arr6; + `checkh(ans, bit6); + ans_enum = enum_t'({ << bit[5:0] {arr6} }); `checkh(ans_enum, bit6); diff --git a/test_regress/t/t_stream_unpack_lhs.out b/test_regress/t/t_stream_unpack_lhs.out index 6203e55f2..b7c7ae776 100644 --- a/test_regress/t/t_stream_unpack_lhs.out +++ b/test_regress/t/t_stream_unpack_lhs.out @@ -1,17 +1,3 @@ -%Warning-WIDTHEXPAND: t/t_stream_unpack_lhs.v:66:29: Operator ASSIGN expects 32 bits on the Assign RHS, but Assign RHS's VARREF 'unpacked_siz_din' generates 8 bits. - : ... note: In instance 't' - 66 | {>>{packed_siz_dout}} = unpacked_siz_din; - | ^ - ... For warning description see https://verilator.org/warn/WIDTHEXPAND?v=latest - ... Use "/* verilator lint_off WIDTHEXPAND */" and lint_on around source to disable this message. -%Warning-WIDTHEXPAND: t/t_stream_unpack_lhs.v:67:29: Operator ASSIGN expects 32 bits on the Assign RHS, but Assign RHS's VARREF 'unpacked_asc_din' generates 8 bits. - : ... note: In instance 't' - 67 | {>>{packed_asc_dout}} = unpacked_asc_din; - | ^ -%Warning-WIDTHEXPAND: t/t_stream_unpack_lhs.v:68:29: Operator ASSIGN expects 32 bits on the Assign RHS, but Assign RHS's VARREF 'unpacked_des_din' generates 8 bits. - : ... note: In instance 't' - 68 | {>>{packed_des_dout}} = unpacked_des_din; - | ^ %Error-UNSUPPORTED: t/t_stream_unpack_lhs.v:113:38: Unsupported/Illegal: Assignment pattern member not underneath a supported construct: NEQ : ... note: In instance 't' 113 | if (unpacked_siz_dout != '{8'h01, 8'h23, 8'h45, 8'h67}) $stop; diff --git a/test_regress/t/t_stream_unpack_narrower.out b/test_regress/t/t_stream_unpack_narrower.out index b3e737b29..80b94fc59 100644 --- a/test_regress/t/t_stream_unpack_narrower.out +++ b/test_regress/t/t_stream_unpack_narrower.out @@ -1,13 +1,21 @@ -%Warning-WIDTHEXPAND: t/t_stream_unpack_narrower.v:14:18: Stream target requires 32 bits, but source expression only provides 31 bits (IEEE 1800-2023 11.4.14.3) +%Warning-WIDTHEXPAND: t/t_stream_unpack_narrower.v:15:18: Stream target requires 32 bits, but source expression only provides 31 bits (IEEE 1800-2023 11.4.14.3) : ... note: In instance 't' - 14 | {>>{stream}} = packed_data; + 15 | {>>{stream}} = packed_data; | ^ ... For warning description see https://verilator.org/warn/WIDTHEXPAND?v=latest ... Use "/* verilator lint_off WIDTHEXPAND */" and lint_on around source to disable this message. -%Warning-WIDTHTRUNC: t/t_stream_unpack_narrower.v:15:17: Target fixed size variable (31 bits) is narrower than the stream (32 bits) (IEEE 1800-2023 11.4.14) +%Warning-WIDTHTRUNC: t/t_stream_unpack_narrower.v:16:17: Target fixed size variable (31 bits) is narrower than the stream (32 bits) (IEEE 1800-2023 11.4.14) : ... note: In instance 't' - 15 | packed_data = {>>{stream}}; + 16 | packed_data = {>>{stream}}; | ^ ... For warning description see https://verilator.org/warn/WIDTHTRUNC?v=latest ... Use "/* verilator lint_off WIDTHTRUNC */" and lint_on around source to disable this message. +%Warning-WIDTHTRUNC: t/t_stream_unpack_narrower.v:17:12: Target fixed size variable (32 bits) is narrower than the stream (61 bits) (IEEE 1800-2023 11.4.14) + : ... note: In instance 't' + 17 | stream = {>>{packed_data2}}; + | ^ +%Warning-WIDTHEXPAND: t/t_stream_unpack_narrower.v:18:24: Stream target requires 61 bits, but source expression only provides 32 bits (IEEE 1800-2023 11.4.14.3) + : ... note: In instance 't' + 18 | {>>{packed_data2}} = stream; + | ^ %Error: Exiting due to diff --git a/test_regress/t/t_stream_unpack_narrower.v b/test_regress/t/t_stream_unpack_narrower.v index 7f7cb4867..f4c49f250 100644 --- a/test_regress/t/t_stream_unpack_narrower.v +++ b/test_regress/t/t_stream_unpack_narrower.v @@ -7,12 +7,15 @@ module t; logic [30:0] packed_data; + logic [60:0] packed_data2; logic [7:0] stream[4]; initial begin packed_data = 31'h12345678; {>>{stream}} = packed_data; packed_data = {>>{stream}}; + stream = {>>{packed_data2}}; + {>>{packed_data2}} = stream; end endmodule