From ec4eb18846cf6fbe7edf7ee886ee3355df33ebfb Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Mon, 21 Jun 2021 07:28:39 +0900 Subject: [PATCH] Fiix incorrect result by bit tree opt (#3023) (#3030) * Add a test to reproduce #3023. Also applied verilog-mode formatting. * use unique_ptr. No functional change is intended. * Introduce restorer that reverts changes during iterate() if failed. --- src/V3Const.cpp | 107 ++++++++++++++++++++++-------- test_regress/t/t_const_opt_red.pl | 2 +- test_regress/t/t_const_opt_red.v | 85 +++++++++++++++++++++--- 3 files changed, 157 insertions(+), 37 deletions(-) diff --git a/src/V3Const.cpp b/src/V3Const.cpp index af4b14b3a..41bcda7cc 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -82,13 +82,58 @@ public: class ConstBitOpTreeVisitor final : public AstNVisitor { // TYPES - struct LeafInfo { // Leaf node (either AstConst or AstVarRef) + struct LeafInfo final { // Leaf node (either AstConst or AstVarRef) bool m_polarity = true; int m_lsb = 0; int m_wordIdx = -1; // -1 means AstWordSel is not used. AstVarRef* m_refp = nullptr; AstConst* m_constp = nullptr; }; + + struct BitPolarityEntry final { // Found bit polarity during iterate() + LeafInfo m_info; + bool m_polarity; + int m_bit; + BitPolarityEntry(const LeafInfo& info, bool pol, int bit) + : m_info(info) + , m_polarity(pol) + , m_bit(bit) {} + BitPolarityEntry() = default; + }; + + class Restorer final { // Restore the original state unless disableRestore() is called + ConstBitOpTreeVisitor& m_visitor; + const size_t m_polaritiesSize; + const size_t m_frozenSize; + const int m_ops; + const bool m_polarity; + bool m_restore; + + public: + explicit Restorer(ConstBitOpTreeVisitor& visitor) + : m_visitor(visitor) + , m_polaritiesSize(visitor.m_bitPolarities.size()) + , m_frozenSize(visitor.m_frozenNodes.size()) + , m_ops(visitor.m_ops) + , m_polarity(visitor.m_polarity) + , m_restore(true) {} + ~Restorer() { + UASSERT(m_visitor.m_bitPolarities.size() >= m_polaritiesSize, + "m_bitPolarities must grow monotorilaclly"); + UASSERT(m_visitor.m_frozenNodes.size() >= m_frozenSize, + "m_frozenNodes must grow monotorilaclly"); + if (m_restore) restoreNow(); + } + void disableRestore() { m_restore = false; } + void restoreNow() { + UASSERT(m_restore, "Can be called just once"); + m_visitor.m_bitPolarities.resize(m_polaritiesSize); + m_visitor.m_frozenNodes.resize(m_frozenSize); + m_visitor.m_ops = m_ops; + m_visitor.m_polarity = m_polarity; + m_restore = false; + } + }; // Collect information for each Variable to transform as below class VarInfo final { // MEMBERS @@ -195,7 +240,8 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { AstUser4InUse m_inuser4; std::vector m_frozenNodes; // Nodes that cannot be optimized - std::vector m_varInfos; // VarInfo for each variable, [0] is nullptr + std::vector m_bitPolarities; // Polarity of bits found during iterate() + std::vector> m_varInfos; // VarInfo for each variable, [0] is nullptr // NODE STATE // AstVarRef::user4u -> Base index of m_varInfos that points VarInfo @@ -235,14 +281,14 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { baseIdx = m_varInfos.size(); const int numWords = ref.m_refp->dtypep()->isWide() ? ref.m_refp->dtypep()->widthWords() : 1; - m_varInfos.resize(m_varInfos.size() + numWords, nullptr); + m_varInfos.resize(m_varInfos.size() + numWords); nodep->user4(baseIdx); } const size_t idx = baseIdx + std::max(0, ref.m_wordIdx); - VarInfo* varInfop = m_varInfos[idx]; + VarInfo* varInfop = m_varInfos[idx].get(); if (!varInfop) { varInfop = new VarInfo{this, ref.m_refp}; - m_varInfos[idx] = varInfop; + m_varInfos[idx].reset(varInfop); } else { if (!varInfop->sameVarAs(ref.m_refp)) CONST_BITOP_SET_FAILED("different var (scope?)", ref.m_refp); @@ -325,6 +371,7 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { } virtual void visit(AstRedXor* nodep) override { // Expect '^(mask & v)' + Restorer restorer{*this}; CONST_BITOP_RETURN_IF(!VN_IS(m_rootp, Xor), nodep); AstAnd* andp = VN_CAST(nodep->lhsp(), And); CONST_BITOP_RETURN_IF(!andp, nodep->lhsp()); @@ -335,13 +382,14 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { LeafInfo leaf = findLeaf(andp->rhsp(), false); CONST_BITOP_RETURN_IF(!leaf.m_refp, andp->rhsp()); + restorer.disableRestore(); // Now all subtree succeeded + incrOps(nodep, __LINE__); incrOps(andp, __LINE__); const V3Number& maskNum = mask.m_constp->num(); - VarInfo& varInfo = getVarInfo(leaf); for (int i = 0; i < maskNum.width(); ++i) { // Set true, m_treePolarity takes care of the entire parity - if (maskNum.bitIs1(i)) varInfo.setPolarity(true, i + leaf.m_lsb); + if (maskNum.bitIs1(i)) m_bitPolarities.emplace_back(leaf, true, i + leaf.m_lsb); } } @@ -357,29 +405,28 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { VL_RESTORER(m_leafp); for (int i = 0; i < 2; ++i) { + Restorer restorer{*this}; LeafInfo leafInfo; m_leafp = &leafInfo; m_curOpp = i == 0 ? nodep->lhsp() : nodep->rhsp(); - const size_t origFrozens = m_frozenNodes.size(); - const int origOps = m_ops; const bool origFailed = m_failed; iterate(m_curOpp); if (leafInfo.m_constp || m_failed) { // Rvert changes in leaf - if (m_frozenNodes.size() > origFrozens) m_frozenNodes.resize(origFrozens); + restorer.restoreNow(); m_frozenNodes.push_back(m_curOpp); - m_ops = origOps; m_failed = origFailed; - } else if (leafInfo.m_refp) { - VarInfo& varInfo = getVarInfo(leafInfo); - if (!varInfo.hasConstantResult()) { - varInfo.setPolarity(isXorTree() || leafInfo.m_polarity, leafInfo.m_lsb); - } + continue; } + restorer.disableRestore(); // Now all checks passed + if (leafInfo.m_refp) + m_bitPolarities.emplace_back(leafInfo, isXorTree() || leafInfo.m_polarity, + leafInfo.m_lsb); } return; } else if (VN_IS(m_rootp, Xor) && VN_IS(nodep, Eq) && isConst(nodep->lhsp(), 0) && VN_IS(nodep->rhsp(), And)) { // 0 == (1 & RedXor) + Restorer restorer{*this}; AstAnd* andp = static_cast(nodep->rhsp()); // already checked above CONST_BITOP_RETURN_IF(!isConst(andp->lhsp(), 1), andp->lhsp()); AstRedXor* redXorp = VN_CAST(andp->rhsp(), RedXor); @@ -388,14 +435,21 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { incrOps(andp, __LINE__); m_polarity = !m_polarity; iterate(redXorp); + CONST_BITOP_RETURN_IF(m_failed, redXorp); + + restorer.disableRestore(); // Now all checks passed return; } else if (VN_IS(m_rootp, Xor) && VN_IS(nodep, And) && isConst(nodep->lhsp(), 1) && (VN_IS(nodep->rhsp(), Xor) || VN_IS(nodep->rhsp(), RedXor))) { // 1 & (v[3] ^ v[2]) + Restorer restorer{*this}; incrOps(nodep, __LINE__); iterate(nodep->rhsp()); + CONST_BITOP_RETURN_IF(m_failed, nodep->rhsp()); + restorer.disableRestore(); // Now all checks passed return; } else if ((isAndTree() && VN_IS(nodep, Eq)) || (isOrTree() && VN_IS(nodep, Neq))) { + Restorer restorer{*this}; CONST_BITOP_RETURN_IF(!m_polarity, nodep); const bool maskFlip = isOrTree(); LeafInfo comp = findLeaf(nodep->lhsp(), true); @@ -410,14 +464,14 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { LeafInfo ref = findLeaf(andp->rhsp(), false); CONST_BITOP_RETURN_IF(!ref.m_refp, andp->rhsp()); - VarInfo& varInfo = getVarInfo(ref); + restorer.disableRestore(); // Now all checks passed const V3Number maskNum = mask.m_constp->num(); const V3Number compNum = comp.m_constp->num(); - for (int i = 0; i < maskNum.width() && !varInfo.hasConstantResult(); ++i) { + for (int i = 0; i < maskNum.width(); ++i) { const int bit = i + ref.m_lsb; if (maskNum.bitIs0(i)) continue; - varInfo.setPolarity(compNum.bitIs1(i) ^ maskFlip, bit); + m_bitPolarities.emplace_back(ref, compNum.bitIs1(i) != maskFlip, bit); } incrOps(nodep, __LINE__); incrOps(andp, __LINE__); @@ -440,13 +494,14 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { incrOps(nodep, __LINE__); iterateChildren(nodep); } + for (auto&& entry : m_bitPolarities) { + VarInfo& info = getVarInfo(entry.m_info); + if (info.hasConstantResult()) continue; + info.setPolarity(entry.m_polarity, entry.m_bit); + } UASSERT_OBJ(isXorTree() || m_polarity, nodep, "must be the original polarity"); } - virtual ~ConstBitOpTreeVisitor() { - for (size_t i = 0; i < m_varInfos.size(); ++i) { - VL_DO_DANGLING(delete m_varInfos[i], m_varInfos[i]); - } - } + virtual ~ConstBitOpTreeVisitor() = default; #undef CONST_BITOP_RETURN_IF #undef CONST_BITOP_SET_FAILED @@ -466,7 +521,7 @@ public: // Two ops for each varInfo. (And and Eq) const int vars = visitor.m_varInfos.size() - 1; int constTerms = 0; - for (const VarInfo* v : visitor.m_varInfos) { + for (auto&& v : visitor.m_varInfos) { if (v && v->hasConstantResult()) ++constTerms; } // Expected number of ops after this simplification @@ -486,7 +541,7 @@ public: AstNode* resultp = nullptr; // VarInfo in visitor.m_varInfos appears in deterministic order, // so the optimized AST is deterministic too. - for (const VarInfo* varinfop : visitor.m_varInfos) { + for (auto&& varinfop : visitor.m_varInfos) { if (!varinfop) continue; AstNode* partialresultp = varinfop->getResult(); resultp = visitor.combineTree(resultp, partialresultp); diff --git a/test_regress/t/t_const_opt_red.pl b/test_regress/t/t_const_opt_red.pl index 4cc2b40a0..d2a4c777b 100755 --- a/test_regress/t/t_const_opt_red.pl +++ b/test_regress/t/t_const_opt_red.pl @@ -19,7 +19,7 @@ execute( ); if ($Self->{vlt}) { - file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 141); + file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 149); } ok(1); diff --git a/test_regress/t/t_const_opt_red.v b/test_regress/t/t_const_opt_red.v index 1f5e407d7..b8d2725ac 100644 --- a/test_regress/t/t_const_opt_red.v +++ b/test_regress/t/t_const_opt_red.v @@ -20,6 +20,8 @@ module t(/*AUTOARG*/ /*AUTOWIRE*/ // Beginning of automatic wires (for undeclared instantiated-module outputs) logic a1; // From test of Test.v + logic a10; // From test of Test.v + logic a11; // From test of Test.v logic a2; // From test of Test.v logic a3; // From test of Test.v logic a4; // From test of Test.v @@ -28,9 +30,9 @@ module t(/*AUTOARG*/ logic a7; // From test of Test.v logic a8; // From test of Test.v logic a9; // From test of Test.v - logic a10; // From test of Test.v - logic a11; // From test of Test.v logic o1; // From test of Test.v + logic o10; // From test of Test.v + logic o11; // From test of Test.v logic o2; // From test of Test.v logic o3; // From test of Test.v logic o4; // From test of Test.v @@ -39,8 +41,6 @@ module t(/*AUTOARG*/ logic o7; // From test of Test.v logic o8; // From test of Test.v logic o9; // From test of Test.v - logic o10; // From test of Test.v - logic o11; // From test of Test.v logic x1; // From test of Test.v logic x2; // From test of Test.v logic x3; // From test of Test.v @@ -59,7 +59,12 @@ module t(/*AUTOARG*/ logic z7; // From test of Test.v // End of automatics - wire [31:0] i = crc[31:0]; + wire [15:0] vec_i = crc[15:0]; + wire [31:0] i = crc[31:0]; + logic b8_i; + logic b12_i; + logic match1_o; + logic match2_o; Test test(/*AUTOINST*/ // Outputs @@ -105,6 +110,15 @@ module t(/*AUTOARG*/ .clk (clk), .i (i[31:0])); + match i_match( + .vec_i ( i[15:0] ), + .b8_i ( b8_i ), + .b12_i ( b12_i ), + .match1_o ( match1_o ), + .match2_o ( match2_o ) + ); + + // Aggregate outputs into a single result vector // verilator lint_off WIDTH wire [63:0] result = {a1,a2,a3,a4,a5,a6,a7,a8,a9,a10,a11, @@ -127,6 +141,8 @@ module t(/*AUTOARG*/ // Setup crc <= 64'h5aef0c8d_d70a4497; sum <= '0; + b8_i <= 1'b0; + b12_i <= 1'b0; end else if (cyc < 10) begin sum <= '0; @@ -157,6 +173,10 @@ module t(/*AUTOARG*/ if (z5 != '1) $stop; if (z6 != '1) $stop; if (z7 != '0) $stop; + if (match1_o != match2_o) begin + $write("[%0t] cyc==%0d m1=%d != m2=%d\n",$time, cyc, match1_o, match2_o); + $stop; + end end else begin $write("[%0t] cyc==%0d crc=%x sum=%x\n",$time, cyc, crc, sum); @@ -173,10 +193,9 @@ endmodule module Test(/*AUTOARG*/ // Outputs - a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, - o1, o2, o3, o4, o5, o6, o7, o8, o9, o10, o11, - x1, x2, x3, x4, x5, x6, x7, x8, x9, - z1, z2, z3, z4, z5, z6, z7, + a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, o1, o2, o3, o4, o5, + o6, o7, o8, o9, o10, o11, x1, x2, x3, x4, x5, x6, x7, x8, x9, z1, + z2, z3, z4, z5, z6, z7, // Inputs clk, i ); @@ -190,7 +209,7 @@ module Test(/*AUTOARG*/ output logic z1, z2, z3, z4, z5, z6, z7; logic [127:0] d; - logic [17:0] e; + logic [17:0] e; always_ff @(posedge clk) d <= {i, ~i, ~i, i}; always_ff @(posedge clk) e <= i[17:00]; @@ -240,3 +259,49 @@ module Test(/*AUTOARG*/ end endmodule + + +module match + ( + input logic [15:0] vec_i, + input logic b8_i, + input logic b12_i, + output logic match1_o, + output logic match2_o + ); + + always_comb + begin + match1_o = 1'b0; + if ( + (vec_i[1:0] == 2'b0) + && + (vec_i[4] == 1'b0) + && + (vec_i[8] == b8_i) + && + (vec_i[12] == b12_i) + ) + begin + match1_o = 1'b1; + end + end + + always_comb + begin + match2_o = 1'b0; + if ( + (vec_i[1:0] == 2'b0) + && + (vec_i[8] == b8_i) + && + (vec_i[12] == b12_i) + && + (vec_i[4] == 1'b0) + ) + begin + match2_o = 1'b1; + end + end + +endmodule