From 9f37cef1bb5d3f87b8873abbfec69b3773b15f96 Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Wed, 6 Jul 2022 08:33:37 +0900 Subject: [PATCH] Fix #3470 of incorrect bit op tree optimization (#3476) * Tests: Add a test to reproduce #3470 * Update LSB during return path of traversal. No functional change is intended. * Introduce LeafInfo::m_msb * Update LeafInfo::m_msb when visitin AstCCast * Internals: Add comment, reorder. No functional change is intended. * Delete explicit from copy constructor to fix build error. * Update Changes * Internals: Remove unused parameter. No functional change is intended. * Tests: Add explanation to t_const_opt. --- Changes | 2 +- src/V3Const.cpp | 61 ++++++++++++++++++++++++----------- test_regress/t/t_const_opt.pl | 2 +- test_regress/t/t_const_opt.v | 36 +++++++++++++++++++-- 4 files changed, 79 insertions(+), 22 deletions(-) diff --git a/Changes b/Changes index b597d565f..2e1183c7b 100644 --- a/Changes +++ b/Changes @@ -13,7 +13,7 @@ Verilator 4.225 devel **Minor:** - +* Fix incorrect bit op tree optimization (#3470). [algrobman] Verilator 4.224 2022-06-19 ========================== diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 7dae3f014..700a99e38 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -80,30 +80,48 @@ class ConstBitOpTreeVisitor final : public VNVisitor { using ResultTerm = std::tuple; class LeafInfo final { // Leaf node (either AstConst or AstVarRef) + // MEMBERS bool m_polarity = true; - int m_lsb = 0; + int m_lsb = 0; // LSB of actually used bit of m_refp->varp() + int m_msb = 0; // MSB of actually used bit of m_refp->varp() int m_wordIdx = -1; // -1 means AstWordSel is not used. AstVarRef* m_refp = nullptr; const AstConst* m_constp = nullptr; public: + // CONSTRUCTORS + LeafInfo() = default; + LeafInfo(const LeafInfo& other) = default; + explicit LeafInfo(int lsb) + : m_lsb{lsb} {} + + // METHODS void setLeaf(AstVarRef* refp) { UASSERT(!m_refp && !m_constp, "Must be called just once"); m_refp = refp; + m_msb = refp->varp()->widthMin() - 1; } void setLeaf(const AstConst* constp) { UASSERT(!m_refp && !m_constp, "Must be called just once"); m_constp = constp; + m_msb = constp->widthMin() - 1; } + void updateBitRange(const AstCCast* castp) { + m_msb = std::min(m_msb, m_lsb + castp->width() - 1); + } + void updateBitRange(const AstShiftR* shiftp) { + m_lsb += VN_AS(shiftp->rhsp(), Const)->toUInt(); + } + void wordIdx(int i) { m_wordIdx = i; } + void polarity(bool p) { m_polarity = p; } + AstVarRef* refp() const { return m_refp; } const AstConst* constp() const { return m_constp; } int wordIdx() const { return m_wordIdx; } bool polarity() const { return m_polarity; } int lsb() const { return m_lsb; } - void wordIdx(int i) { m_wordIdx = i; } - void lsb(int l) { m_lsb = l; } - void polarity(bool p) { m_polarity = p; } + int msb() const { return std::min(m_msb, varWidth() - 1); } int varWidth() const { UASSERT(m_refp, "m_refp should be set"); const int width = m_refp->varp()->widthMin(); @@ -382,7 +400,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { // Traverse down to see AstConst or AstVarRef LeafInfo findLeaf(AstNode* nodep, bool expectConst) { - LeafInfo info; + LeafInfo info{m_lsb}; { VL_RESTORER(m_leafp); m_leafp = &info; @@ -402,7 +420,10 @@ class ConstBitOpTreeVisitor final : public VNVisitor { virtual void visit(AstNode* nodep) override { CONST_BITOP_SET_FAILED("Hit unexpected op", nodep); } - virtual void visit(AstCCast* nodep) override { iterateChildren(nodep); } + virtual void visit(AstCCast* nodep) override { + iterateChildren(nodep); + if (m_leafp) m_leafp->updateBitRange(nodep); + } virtual void visit(AstShiftR* nodep) override { CONST_BITOP_RETURN_IF(!m_leafp, nodep); AstConst* const constp = VN_CAST(nodep->rhsp(), Const); @@ -410,12 +431,14 @@ class ConstBitOpTreeVisitor final : public VNVisitor { m_lsb += constp->toUInt(); incrOps(nodep, __LINE__); iterate(nodep->lhsp()); + m_leafp->updateBitRange(nodep); m_lsb -= constp->toUInt(); } virtual void visit(AstNot* nodep) override { CONST_BITOP_RETURN_IF(nodep->widthMin() != 1, nodep); AstNode* lhsp = nodep->lhsp(); - if (AstCCast* const castp = VN_CAST(lhsp, CCast)) lhsp = castp->lhsp(); + AstCCast* const castp = VN_CAST(lhsp, CCast); + if (castp) lhsp = castp->lhsp(); CONST_BITOP_RETURN_IF(!VN_IS(lhsp, VarRef) && !VN_IS(lhsp, Xor) && !VN_IS(lhsp, RedXor) && !VN_IS(lhsp, ShiftR), lhsp); @@ -424,6 +447,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { iterateChildren(nodep); // Don't restore m_polarity for Xor as it counts parity of the entire tree if (!isXorTree()) m_polarity = !m_polarity; + if (m_leafp && castp) m_leafp->updateBitRange(castp); } virtual void visit(AstWordSel* nodep) override { CONST_BITOP_RETURN_IF(!m_leafp, nodep); @@ -437,27 +461,27 @@ class ConstBitOpTreeVisitor final : public VNVisitor { CONST_BITOP_RETURN_IF(!m_leafp, nodep); m_leafp->setLeaf(nodep); m_leafp->polarity(m_polarity); - m_leafp->lsb(m_lsb); } virtual void visit(AstConst* nodep) override { CONST_BITOP_RETURN_IF(!m_leafp, nodep); m_leafp->setLeaf(nodep); - m_leafp->lsb(m_lsb); } virtual void visit(AstRedXor* nodep) override { Restorer restorer{*this}; CONST_BITOP_RETURN_IF(!VN_IS(m_rootp, Xor), nodep); AstNode* lhsp = nodep->lhsp(); - if (const AstCCast* const castp = VN_CAST(lhsp, CCast)) lhsp = castp->lhsp(); + const AstCCast* const castp = VN_CAST(lhsp, CCast); + if (castp) lhsp = castp->lhsp(); if (const AstAnd* const andp = VN_CAST(lhsp, And)) { // '^(mask & leaf)' CONST_BITOP_RETURN_IF(!andp, lhsp); const LeafInfo& mask = findLeaf(andp->lhsp(), true); CONST_BITOP_RETURN_IF(!mask.constp() || mask.lsb() != 0, andp->lhsp()); - const LeafInfo& ref = findLeaf(andp->rhsp(), false); + LeafInfo ref = findLeaf(andp->rhsp(), false); CONST_BITOP_RETURN_IF(!ref.refp(), andp->rhsp()); + if (castp) ref.updateBitRange(castp); restorer.disableRestore(); // Now all subtree succeeded @@ -467,7 +491,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { incrOps(andp, __LINE__); // Mark all bits checked in this reduction - const int maxBitIdx = std::min(ref.lsb() + maskNum.width(), ref.varWidth()); + const int maxBitIdx = std::min(ref.lsb() + maskNum.width(), ref.msb() + 1); for (int bitIdx = ref.lsb(); bitIdx < maxBitIdx; ++bitIdx) { const int maskIdx = bitIdx - ref.lsb(); if (maskNum.bitIs0(maskIdx)) continue; @@ -475,15 +499,16 @@ class ConstBitOpTreeVisitor final : public VNVisitor { m_bitPolarities.emplace_back(ref, true, bitIdx); } } else { // '^leaf' - const LeafInfo& ref = findLeaf(lhsp, false); + LeafInfo ref = findLeaf(lhsp, false); CONST_BITOP_RETURN_IF(!ref.refp(), lhsp); + if (castp) ref.updateBitRange(castp); restorer.disableRestore(); // Now all checks passed incrOps(nodep, __LINE__); // Mark all bits checked by this comparison - for (int bitIdx = ref.lsb(); bitIdx < ref.varWidth(); ++bitIdx) { + for (int bitIdx = ref.lsb(); bitIdx <= ref.msb(); ++bitIdx) { m_bitPolarities.emplace_back(ref, true, bitIdx); } } @@ -503,7 +528,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { for (const bool right : {false, true}) { Restorer restorer{*this}; - LeafInfo leafInfo; + LeafInfo leafInfo{m_lsb}; m_leafp = &leafInfo; AstNode* opp = right ? nodep->rhsp() : nodep->lhsp(); const bool origFailed = m_failed; @@ -522,7 +547,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { // The conditional on the lsb being in range is necessary for some degenerate // case, e.g.: (IData)((QData)wide[0] >> 32), or <1-bit-var> >> 1, which is // just zero - if (leafInfo.lsb() < leafInfo.varWidth()) { + if (leafInfo.lsb() <= leafInfo.msb()) { m_bitPolarities.emplace_back(leafInfo, isXorTree() || leafInfo.polarity(), leafInfo.lsb()); } else if (isAndTree() && leafInfo.polarity()) { @@ -559,7 +584,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { incrOps(andp, __LINE__); // Mark all bits checked by this comparison - const int maxBitIdx = std::min(ref.lsb() + maskNum.width(), ref.varWidth()); + const int maxBitIdx = std::min(ref.lsb() + maskNum.width(), ref.msb() + 1); for (int bitIdx = ref.lsb(); bitIdx < maxBitIdx; ++bitIdx) { const int maskIdx = bitIdx - ref.lsb(); if (maskNum.bitIs0(maskIdx)) continue; @@ -575,7 +600,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { incrOps(nodep, __LINE__); // Mark all bits checked by this comparison - const int maxBitIdx = std::min(ref.lsb() + compNum.width(), ref.varWidth()); + const int maxBitIdx = std::min(ref.lsb() + compNum.width(), ref.msb() + 1); for (int bitIdx = ref.lsb(); bitIdx < maxBitIdx; ++bitIdx) { const int maskIdx = bitIdx - ref.lsb(); const bool polarity = compNum.bitIs1(maskIdx) != maskFlip; diff --git a/test_regress/t/t_const_opt.pl b/test_regress/t/t_const_opt.pl index 83e301744..837b5f74f 100755 --- a/test_regress/t/t_const_opt.pl +++ b/test_regress/t/t_const_opt.pl @@ -19,7 +19,7 @@ execute( ); if ($Self->{vlt}) { - file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 11); + file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 14); } ok(1); 1; diff --git a/test_regress/t/t_const_opt.v b/test_regress/t/t_const_opt.v index 407fef13c..e24d28bf8 100644 --- a/test_regress/t/t_const_opt.v +++ b/test_regress/t/t_const_opt.v @@ -62,7 +62,7 @@ module t(/*AUTOARG*/ $write("[%0t] cyc==%0d crc=%x sum=%x\n", $time, cyc, crc, sum); if (crc !== 64'hc77bb9b3784ea091) $stop; // What checksum will we end up with (above print should match) -`define EXPECTED_SUM 64'hdccb9e7b8b638233 +`define EXPECTED_SUM 64'hde21e019a3e12039 if (sum !== `EXPECTED_SUM) $stop; $write("*-* All Finished *-*\n"); @@ -86,10 +86,11 @@ module Test(/*AUTOARG*/ logic bug3182_out; logic bug3197_out; logic bug3445_out; + logic bug3470_out; output logic o; - logic [7:0] tmp; + logic [8:0] tmp; assign o = ^tmp; always_ff @(posedge clk) begin @@ -113,11 +114,13 @@ module Test(/*AUTOARG*/ tmp[5] <= bug3182_out; tmp[6] <= bug3197_out; tmp[7] <= bug3445_out; + tmp[8] <= bug3470_out; end bug3182 i_bug3182(.in(d[4:0]), .out(bug3182_out)); bug3197 i_bug3197(.clk(clk), .in(d), .out(bug3197_out)); bug3445 i_bug3445(.clk(clk), .in(d), .out(bug3445_out)); + bug3470 i_bug3470(.clk(clk), .in(d), .out(bug3470_out)); endmodule @@ -203,3 +206,32 @@ module bug3445(input wire clk, input wire [31:0] in, output wire out); assign out = result0 ^ result1 ^ (result2 | result3); endmodule + +// Bug3470 +// CCast had been ignored in bit op tree optimization +// Assume the following HDL input: +// (^d[38:32]) ^ (^d[31:0]) +// where d is logic [38:0] +// ^d[31:0] becomes REDXOR(CCast(uint32_t, d)), +// but CCast was ignored and interpreted as ^d[38:0]. +// Finally (^d[38:32]) ^ (^d31:0]) was wrongly transformed to +// (^d[38:32]) ^ (^d[38:0]) +// -> (^d[38:32]) ^ ((^d[38:32]) ^ (^d[31:0])) +// -> ^d[31:0] +// Of course the correct result is ^d[38:0] = ^d +module bug3470(input wire clk, input wire [31:0] in, output wire out); + logic [38:0] d; + always_ff @(posedge clk) + d <= {d[6:0], in}; + + logic tmp, expected; + always_ff @(posedge clk) begin + tmp <= ^(d >> 32) ^ (^d[31:0]); + expected <= ^d; + end + + always @(posedge clk) + if (tmp != expected) $stop; + + assign out = tmp; +endmodule