From 6ae6b16223ebaf03f3fef418ee1bd69fd758fe9c Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 5 Nov 2022 11:35:42 +0000 Subject: [PATCH] V3Const: Fix folding of LogAnd with non-bool operands Folding an AstLogAnd with a non-zero constant operand used to coerce the type of the other operand, yielding an ill-typed node that DFG was then unhappy about. Add a RedOr instead if the width of the replacement operand is greater than zero. Fixes #3726 --- src/V3Const.cpp | 24 ++++++++++++++++++------ test_regress/t/t_dfg_3726.pl | 16 ++++++++++++++++ test_regress/t/t_dfg_3726.v | 19 +++++++++++++++++++ 3 files changed, 53 insertions(+), 6 deletions(-) create mode 100755 test_regress/t/t_dfg_3726.pl create mode 100644 test_regress/t/t_dfg_3726.v diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 3f75f93ae..08ad3272d 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -1646,6 +1646,16 @@ private: nodep->replaceWith(childp); VL_DO_DANGLING(nodep->deleteTree(), nodep); } + void replaceWChildBool(AstNode* nodep, AstNode* childp) { + // NODE(..., CHILD(...)) -> REDOR(CHILD(...)) + childp->unlinkFrBack(); + if (childp->width1()) { + nodep->replaceWith(childp); + } else { + nodep->replaceWith(new AstRedOr{childp->fileline(), childp}); + } + VL_DO_DANGLING(nodep->deleteTree(), nodep); + } //! Replace a ternary node with its RHS after iterating //! Used with short-circuiting, where the RHS has not yet been iterated. @@ -1672,6 +1682,8 @@ private: // Keep RHS, remove LHS replaceWChild(nodep, nodep->rhsp()); } + void replaceWLhsBool(AstNodeBiop* nodep) { replaceWChildBool(nodep, nodep->lhsp()); } + void replaceWRhsBool(AstNodeBiop* nodep) { replaceWChildBool(nodep, nodep->rhsp()); } void replaceAsv(AstNodeBiop* nodep) { // BIASV(CONSTa, BIASV(CONSTb, c)) -> BIASV( BIASV_CONSTED(a,b), c) // BIASV(SAMEa, BIASV(SAMEb, c)) -> BIASV( BIASV(SAMEa,SAMEb), c) @@ -3282,7 +3294,7 @@ private: TREEOP ("AstLogAnd{$lhsp.isZero, $rhsp}", "replaceZero(nodep)"); // This visit function here must allow for short-circuiting. TREEOPS("AstLogOr {$lhsp.isOne}", "replaceNum(nodep, 1)"); - TREEOP ("AstLogOr {$lhsp.isZero, $rhsp}", "replaceWRhs(nodep)"); + TREEOP ("AstLogOr {$lhsp.isZero, $rhsp}", "replaceWRhsBool(nodep)"); TREEOP ("AstDiv {$lhsp.isZero, $rhsp}", "replaceZeroChkPure(nodep,$rhsp)"); TREEOP ("AstDivS {$lhsp.isZero, $rhsp}", "replaceZeroChkPure(nodep,$rhsp)"); TREEOP ("AstMul {$lhsp.isZero, $rhsp}", "replaceZeroChkPure(nodep,$rhsp)"); @@ -3304,7 +3316,7 @@ private: TREEOP ("AstAdd {$lhsp, $rhsp.isZero}", "replaceWLhs(nodep)"); TREEOP ("AstAnd {$lhsp, $rhsp.isZero}", "replaceZeroChkPure(nodep,$lhsp)"); TREEOP ("AstLogAnd{$lhsp, $rhsp.isZero}", "replaceZeroChkPure(nodep,$lhsp)"); - TREEOP ("AstLogOr {$lhsp, $rhsp.isZero}", "replaceWLhs(nodep)"); + TREEOP ("AstLogOr {$lhsp, $rhsp.isZero}", "replaceWLhsBool(nodep)"); TREEOP ("AstMul {$lhsp, $rhsp.isZero}", "replaceZeroChkPure(nodep,$lhsp)"); TREEOP ("AstMulS {$lhsp, $rhsp.isZero}", "replaceZeroChkPure(nodep,$lhsp)"); TREEOP ("AstOr {$lhsp, $rhsp.isZero}", "replaceWLhs(nodep)"); @@ -3315,11 +3327,11 @@ private: TREEOP ("AstXor {$lhsp, $rhsp.isZero}", "replaceWLhs(nodep)"); // Non-zero on one side or the other TREEOP ("AstAnd {$lhsp.isAllOnes, $rhsp}", "replaceWRhs(nodep)"); - TREEOP ("AstLogAnd{$lhsp.isNeqZero, $rhsp}", "replaceWRhs(nodep)"); + TREEOP ("AstLogAnd{$lhsp.isNeqZero, $rhsp}", "replaceWRhsBool(nodep)"); TREEOP ("AstOr {$lhsp.isAllOnes, $rhsp, isTPure($rhsp)}", "replaceWLhs(nodep)"); // ->allOnes TREEOP ("AstLogOr {$lhsp.isNeqZero, $rhsp}", "replaceNum(nodep,1)"); TREEOP ("AstAnd {$lhsp, $rhsp.isAllOnes}", "replaceWLhs(nodep)"); - TREEOP ("AstLogAnd{$lhsp, $rhsp.isNeqZero}", "replaceWLhs(nodep)"); + TREEOP ("AstLogAnd{$lhsp, $rhsp.isNeqZero}", "replaceWLhsBool(nodep)"); TREEOP ("AstOr {$lhsp, $rhsp.isAllOnes, isTPure($lhsp)}", "replaceWRhs(nodep)"); // ->allOnes TREEOP ("AstLogOr {$lhsp, $rhsp.isNeqZero, isTPure($lhsp), nodep->isPure()}", "replaceNum(nodep,1)"); TREEOP ("AstXor {$lhsp.isAllOnes, $rhsp}", "AstNot{$rhsp}"); @@ -3460,8 +3472,8 @@ private: TREEOP ("AstNeqN {operandsSame($lhsp,,$rhsp)}", "replaceZero(nodep)"); TREEOP ("AstNeqCase{operandsSame($lhsp,,$rhsp)}", "replaceZero(nodep)"); TREEOP ("AstNeqWild{operandsSame($lhsp,,$rhsp)}", "replaceZero(nodep)"); - TREEOP ("AstLogAnd {operandsSame($lhsp,,$rhsp), $lhsp.width1}", "replaceWLhs(nodep)"); - TREEOP ("AstLogOr {operandsSame($lhsp,,$rhsp), $lhsp.width1}", "replaceWLhs(nodep)"); + TREEOP ("AstLogAnd {operandsSame($lhsp,,$rhsp)}", "replaceWLhsBool(nodep)"); + TREEOP ("AstLogOr {operandsSame($lhsp,,$rhsp)}", "replaceWLhsBool(nodep)"); ///=== Verilog operators // Comparison against 1'b0/1'b1; must be careful about widths. // These use Not, so must be Verilog only diff --git a/test_regress/t/t_dfg_3726.pl b/test_regress/t/t_dfg_3726.pl new file mode 100755 index 000000000..84ae125be --- /dev/null +++ b/test_regress/t/t_dfg_3726.pl @@ -0,0 +1,16 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2022 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 + +scenarios(vlt => 1); + +compile(); + +ok(1); +1; diff --git a/test_regress/t/t_dfg_3726.v b/test_regress/t/t_dfg_3726.v new file mode 100644 index 000000000..8331d8b95 --- /dev/null +++ b/test_regress/t/t_dfg_3726.v @@ -0,0 +1,19 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Geza Lore. +// SPDX-License-Identifier: CC0-1.0 + +module t(/*AUTOARG*/ + // Outputs + x, + // Inputs + i + ); + + input i; + output x; + + assign x = (i ? 0 : 1) && 1; + +endmodule