diff --git a/Changes b/Changes index aa60688ff..d56469b13 100644 --- a/Changes +++ b/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 diff --git a/src/V3Const.cpp b/src/V3Const.cpp index dcf19c421..530a7936d 100644 --- a/src/V3Const.cpp +++ b/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())"); diff --git a/test_regress/t/t_const_bitoptree_bug3096.cpp b/test_regress/t/t_const_bitoptree_bug3096.cpp new file mode 100644 index 000000000..27a3ca385 --- /dev/null +++ b/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 +#include + +#include + +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(tb->illegal_instr_o) << std::endl + << std::flush; + assert(tb->illegal_instr_o == 0); + + delete tb; + return 0; +} diff --git a/test_regress/t/t_const_bitoptree_bug3096.pl b/test_regress/t/t_const_bitoptree_bug3096.pl new file mode 100755 index 000000000..16e215620 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_const_bitoptree_bug3096.v b/test_regress/t/t_const_bitoptree_bug3096.v new file mode 100644 index 000000000..2f3e42f97 --- /dev/null +++ b/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