Fix bitop tree optimization dropping necessary cleaning AND (#3097)

Fixes #3096.

Changes

@@ -25,6 +25,7 @@ Verilator 4.211 devel
 * Refactored Verilated include files; include verilated.h not verilated_heavy.h.
 * Fix -G to treat simple integer literals as signed (#3060). [Anikin1610]
 * Fix emitted string array initializers (#2895). [Iztok Jeras]
+* Fix bitop tree optimization dropping necessary & operator (#3096). [Flavien Solt]
 
 
 Verilator 4.210 2021-07-07

src/V3Const.cpp

@@ -858,12 +858,23 @@ private:
     }
 
     bool matchBitOpTree(AstNode* nodep) {
+        if (nodep->widthMin() != 1) return false;
         if (!v3Global.opt.oConstBitOpTree()) return false;
 
+        string debugPrefix;
+        if (debug() >= 9) {  // LCOV_EXCL_START
+            static int c = 0;
+            debugPrefix = "matchBitOpTree[";
+            debugPrefix += cvtToStr(++c);
+            debugPrefix += "] ";
+            nodep->dumpTree(debugPrefix + "INPUT: ");
+        }  // LCOV_EXCL_STOP
+
         AstNode* newp = nullptr;
         bool tried = false;
-        if (AstAnd* andp = VN_CAST(nodep, And)) {  // 1 & BitOpTree
-            if (AstConst* bitMaskp = VN_CAST(andp->lhsp(), Const)) {
+        if (AstAnd* const andp = VN_CAST(nodep, And)) {  // 1 & BitOpTree
+            AstConst* const bitMaskp = VN_CAST(andp->lhsp(), Const);
+            if (bitMaskp && andp->rhsp()->widthMin() == 1) {
                 if (bitMaskp->num().toUQuad() != 1) return false;
                 newp = ConstBitOpTreeVisitor::simplify(andp->rhsp(), 1, m_statBitOpReduction);
                 tried = true;
@@ -876,17 +887,16 @@ private:
         }
         if (newp) {
             UINFO(4, "Transformed leaf of bit tree to " << newp << std::endl);
-            if (debug() >= 9) {  // LCOV_EXCL_START
-                static int c = 0;
-                std::cout << "Call matchBitOpTree[" << c << "]\n";
-                nodep->dumpTree(std::cout);
-                std::cout << "\nResult:\n";
-                newp->dumpTree(std::cout);
-                ++c;
-            }  // LCOV_EXCL_STOP
             nodep->replaceWith(newp);
             VL_DO_DANGLING(nodep->deleteTree(), nodep);
         }
+        if (debug() >= 9) {  // LCOV_EXCL_START
+            if (newp) {
+                newp->dumpTree(debugPrefix + "RESULT: ");
+            } else {
+                cout << debugPrefix << "not replaced" << endl;
+            }
+        }  // LCOV_EXCL_STOP
         return newp;
     }
     static bool operandShiftSame(const AstNode* nodep) {
@@ -3101,7 +3111,7 @@ private:
     TREEOPV("AstRedOr {$lhsp.castExtend}",      "AstRedOr {$lhsp->castExtend()->lhsp()}");
     TREEOPV("AstRedXor{$lhsp.castExtend}",      "AstRedXor{$lhsp->castExtend()->lhsp()}");
     TREEOP ("AstRedXor{$lhsp.castXor, VN_IS(VN_CAST($lhsp,,Xor)->lhsp(),,Const)}", "AstXor{AstRedXor{$lhsp->castXor()->lhsp()}, AstRedXor{$lhsp->castXor()->rhsp()}}");  // ^(const ^ a) => (^const)^(^a)
-    TREEOPC("AstAnd {nodep->widthMin() == 1, $lhsp.castConst, $rhsp.castRedXor, matchBitOpTree(nodep)}", "DONE");
+    TREEOPC("AstAnd {$lhsp.castConst, $rhsp.castRedXor, matchBitOpTree(nodep)}", "DONE");
     TREEOPV("AstOneHot{$lhsp.width1}",          "replaceWLhs(nodep)");
     TREEOPV("AstOneHot0{$lhsp.width1}",         "replaceNum(nodep,1)");
     // Binary AND/OR is faster than logical and/or (usually)
@@ -3125,9 +3135,9 @@ private:
     TREEOP ("AstAnd {operandShiftSame(nodep)}",         "replaceShiftSame(nodep)");
     TREEOP ("AstOr  {operandShiftSame(nodep)}",         "replaceShiftSame(nodep)");
     TREEOP ("AstXor {operandShiftSame(nodep)}",         "replaceShiftSame(nodep)");
-    TREEOPC("AstAnd {nodep->widthMin() == 1, matchBitOpTree(nodep)}", "DONE");
-    TREEOPC("AstOr  {nodep->widthMin() == 1, matchBitOpTree(nodep)}", "DONE");
-    TREEOPC("AstXor {nodep->widthMin() == 1, matchBitOpTree(nodep)}", "DONE");
+    TREEOPC("AstAnd {matchBitOpTree(nodep)}", "DONE");
+    TREEOPC("AstOr  {matchBitOpTree(nodep)}", "DONE");
+    TREEOPC("AstXor {matchBitOpTree(nodep)}", "DONE");
     // Note can't simplify a extend{extends}, extends{extend}, as the sign
     // bits end up in the wrong places
     TREEOPV("AstExtend {$lhsp.castExtend}",  "replaceExtend(nodep, VN_CAST(nodep->lhsp(), Extend)->lhsp())");

test_regress/t/t_const_bitoptree_bug3096.cpp

@@ -0,0 +1,29 @@
+// -*- mode: C++; c-file-style: "cc-mode" -*-
+//*************************************************************************
+//
+// Copyright 2021 by Geza Lore. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+//
+//*************************************************************************
+
+#include <cassert>
+#include <iostream>
+
+#include <Vt_const_bitoptree_bug3096.h>
+
+int main(int argc, char* argv[]) {
+    Vt_const_bitoptree_bug3096* const tb = new Vt_const_bitoptree_bug3096;
+
+    tb->instr_i = 0x08c0006f;
+    tb->eval();
+
+    std::cout << "tb->illegal_instr_o: " << static_cast<int>(tb->illegal_instr_o) << std::endl
+              << std::flush;
+    assert(tb->illegal_instr_o == 0);
+
+    delete tb;
+    return 0;
+}

test_regress/t/t_const_bitoptree_bug3096.pl

@@ -0,0 +1,22 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2015 by Todd Strader. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+scenarios(vlt_all => 1);
+
+compile(
+    make_top_shell => 0,
+    make_main => 0,
+    v_flags2 => ["--exe $Self->{t_dir}/$Self->{name}.cpp"],
+    );
+
+execute();
+
+ok(1);
+1;

test_regress/t/t_const_bitoptree_bug3096.v

@@ -0,0 +1,24 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// Copyright 2021 by Geza Lore. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+// From issue #3096
+
+module decoder(
+               input wire [31:0] instr_i,
+               // Making 'a' an output preserves it as a sub-expression and causes a missing clean
+               output wire       a,
+               output wire       illegal_instr_o
+               );
+   /* verilator lint_off WIDTH */
+   wire                          b = ! instr_i[12:5];
+   wire                          c = ! instr_i[1:0];
+   wire                          d = ! instr_i[15:13];
+   /* verilator lint_on WIDTH */
+   assign a = d ? b : 1'h1;
+   assign illegal_instr_o = c ? a : 1'h0;
+endmodule