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

@@ -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<AstNode*> m_frozenNodes;  // Nodes that cannot be optimized
-    std::vector<VarInfo*> m_varInfos;  // VarInfo for each variable, [0] is nullptr
+    std::vector<BitPolarityEntry> m_bitPolarities;  // Polarity of bits found during iterate()
+    std::vector<std::unique_ptr<VarInfo>> 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) {
+                    continue;
-                    VarInfo& varInfo = getVarInfo(leafInfo);
-                    if (!varInfo.hasConstantResult()) {
-                        varInfo.setPolarity(isXorTree() || leafInfo.m_polarity, leafInfo.m_lsb);
-                    }
                 }
+                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<AstAnd*>(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() {
+    virtual ~ConstBitOpTreeVisitor() = default;
-        for (size_t i = 0; i < m_varInfos.size(); ++i) {
-            VL_DO_DANGLING(delete m_varInfos[i], m_varInfos[i]);
-        }
-    }
 #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);

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);

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,
+   a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, o1, o2, o3, o4, o5,
-   o1, o2, o3, o4, o5, o6, o7, o8, o9, o10, o11,
+   o6, o7, o8, o9, o10, o11, x1, x2, x3, x4, x5, x6, x7, x8, x9, z1,
-   x1, x2, x3, x4, x5, x6, x7, x8, x9,
+   z2, z3, z4, z5, z6, z7,
-   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