From 69eb76ad6638a51cb58fe8142c434ad6a40e2667 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Mon, 5 May 2025 07:04:20 -0400 Subject: [PATCH] Fix constant propagation of post-expand stages (#5983). --- Changes | 2 +- docs/internals.rst | 2 +- src/V3Const.cpp | 40 ++++++------- src/V3Expand.cpp | 1 - test_regress/t/t_math_cv_bitop.out | 6 ++ test_regress/t/t_math_cv_bitop.py | 18 ++++++ test_regress/t/t_math_cv_bitop.v | 92 ++++++++++++++++++++++++++++++ 7 files changed, 138 insertions(+), 23 deletions(-) create mode 100644 test_regress/t/t_math_cv_bitop.out create mode 100755 test_regress/t/t_math_cv_bitop.py create mode 100644 test_regress/t/t_math_cv_bitop.v diff --git a/Changes b/Changes index e4a4d1a5a..6404647b4 100644 --- a/Changes +++ b/Changes @@ -17,7 +17,7 @@ Verilator 5.037 devel * Add PROCINITASSIGN on initial assignments to process variables (#2481). [Niraj Menon] * Fix filename backslash escapes in C code (#5947). * Fix C++ widths in V3Expand (#5953) (#5975). [Geza Lore] -* Fix constant propagation of post-expand stages (#5955) (#5963) (#5969) (#5972). +* Fix constant propagation of post-expand stages (#5955) (#5963) (#5969) (#5972) (#5983). * Fix sign extension of signed compared with unsigned case items (#5968). * Fix always processes ignoring $finish (#5971). [Hennadii Chernyshchyk] * Fix streaming to/from packed arrays (#5976). [Geza Lore] diff --git a/docs/internals.rst b/docs/internals.rst index 6825c0c21..acef93bc7 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -1903,7 +1903,7 @@ find what made a line in the tree dumps): :: - watch AstNode::s_editCntGbl==#### + watch AstNode::s_editCntGbl=#### Then, when the watch fires, to break at every following change to that node: diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 864c3a283..96fb5b0a7 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -1022,7 +1022,7 @@ class ConstVisitor final : public VNVisitor { } } if (ccastp) { - andp->replaceWith(ccastp); + andp->replaceWithKeepDType(ccastp); VL_DO_DANGLING(pushDeletep(andp), andp); return true; } @@ -1128,16 +1128,16 @@ class ConstVisitor final : public VNVisitor { const bool orRIsRedundant = checkBottomClear(orp->rhsp()); if (orLIsRedundant && orRIsRedundant) { - nodep->replaceWith( + nodep->replaceWithKeepDType( new AstConst{nodep->fileline(), AstConst::DTyped{}, nodep->dtypep()}); VL_DO_DANGLING(pushDeletep(nodep), nodep); return true; } else if (orLIsRedundant) { - orp->replaceWith(orp->rhsp()->unlinkFrBack()); + orp->replaceWithKeepDType(orp->rhsp()->unlinkFrBack()); VL_DO_DANGLING(pushDeletep(orp), orp); return false; // input node is still valid, keep going } else if (orRIsRedundant) { - orp->replaceWith(orp->lhsp()->unlinkFrBack()); + orp->replaceWithKeepDType(orp->lhsp()->unlinkFrBack()); VL_DO_DANGLING(pushDeletep(orp), orp); return false; // input node is still valid, keep going } else { @@ -1204,8 +1204,8 @@ class ConstVisitor final : public VNVisitor { } if (newp) { + nodep->replaceWithKeepDType(newp); UINFO(4, "Transformed leaf of bit tree to " << newp << std::endl); - nodep->replaceWith(newp); VL_DO_DANGLING(pushDeletep(nodep), nodep); } @@ -1657,7 +1657,7 @@ class ConstVisitor final : public VNVisitor { AstNode* const newp = new AstConst{oldp->fileline(), AstConst::String{}, num}; if (debug() > 5) oldp->dumpTree("- const_old: "); if (debug() > 5) newp->dumpTree("- _new: "); - oldp->replaceWith(newp); + oldp->replaceWithKeepDType(newp); VL_DO_DANGLING(pushDeletep(oldp), oldp); } //---------------------------------------- @@ -1676,9 +1676,9 @@ class ConstVisitor final : public VNVisitor { // NODE(..., CHILD(...)) -> REDOR(CHILD(...)) childp->unlinkFrBack(); if (childp->width1()) { - nodep->replaceWith(childp); + nodep->replaceWithKeepDType(childp); } else { - nodep->replaceWith(new AstRedOr{childp->fileline(), childp}); + nodep->replaceWithKeepDType(new AstRedOr{childp->fileline(), childp}); } VL_DO_DANGLING(pushDeletep(nodep), nodep); } @@ -1767,7 +1767,7 @@ class ConstVisitor final : public VNVisitor { AstNodeBiop* const rp = VN_AS(nodep->rhsp()->unlinkFrBack(), NodeBiop); AstNodeExpr* const rlp = rp->lhsp()->unlinkFrBack(); AstNodeExpr* const rrp = rp->rhsp()->unlinkFrBack(); - nodep->replaceWith(lp); + nodep->replaceWithKeepDType(lp); if (operandsSame(llp, rlp)) { lp->lhsp(llp); lp->rhsp(nodep); @@ -1798,7 +1798,7 @@ class ConstVisitor final : public VNVisitor { AstNodeBiop* const rp = VN_AS(nodep->rhsp()->unlinkFrBack(), NodeBiop); AstNodeExpr* const rlp = rp->lhsp()->unlinkFrBack(); AstNodeExpr* const rrp = rp->rhsp()->unlinkFrBack(); - nodep->replaceWith(lp); + nodep->replaceWithKeepDType(lp); lp->lhsp(nodep); lp->rhsp(lrp); nodep->lhsp(llp); @@ -1824,7 +1824,7 @@ class ConstVisitor final : public VNVisitor { UINFO(5, "merged two adjacent sel " << lselp << " and " << rselp << " to one " << newselp << endl); - nodep->replaceWith(newselp); + nodep->replaceWithKeepDType(newselp); VL_DO_DANGLING(pushDeletep(lselp), lselp); VL_DO_DANGLING(pushDeletep(rselp), rselp); VL_DO_DANGLING(pushDeletep(nodep), nodep); @@ -1850,7 +1850,7 @@ class ConstVisitor final : public VNVisitor { lp->dtypeChgWidthSigned(newlp->width(), newlp->width(), VSigning::UNSIGNED); UINFO(5, "merged " << nodep << endl); VL_DO_DANGLING(pushDeletep(rp->unlinkFrBack()), rp); - nodep->replaceWith(lp->unlinkFrBack()); + nodep->replaceWithKeepDType(lp->unlinkFrBack()); VL_DO_DANGLING(pushDeletep(nodep), nodep); iterate(lp->lhsp()); iterate(lp->rhsp()); @@ -2043,7 +2043,7 @@ class ConstVisitor final : public VNVisitor { new AstConcat{rhs1p->fileline(), rhs1p, rhs2p}); } // if (debug()) pnewp->dumpTree("- conew: "); - nodep->replaceWith(newp); + nodep->replaceWith(newp); // dypep intentionally changing VL_DO_DANGLING(pushDeletep(nodep), nodep); VL_DO_DANGLING(pushDeletep(nextp->unlinkFrBack()), nextp); return true; @@ -2562,7 +2562,7 @@ class ConstVisitor final : public VNVisitor { newlsbp->dtypeFrom(widep); } AstSel* const newp = new AstSel{nodep->fileline(), fromp, newlsbp, widthp}; - nodep->replaceWith(newp); + nodep->replaceWithKeepDType(newp); VL_DO_DANGLING(pushDeletep(nodep), nodep); } @@ -2576,12 +2576,12 @@ class ConstVisitor final : public VNVisitor { AstSel* const newp = new AstSel{nodep->fileline(), conLhsp, nodep->lsbConst() - conRhsp->width(), nodep->widthConst()}; - nodep->replaceWith(newp); + nodep->replaceWithKeepDType(newp); } else if (static_cast(nodep->msbConst()) < conRhsp->width()) { conRhsp->unlinkFrBack(); AstSel* const newp = new AstSel{nodep->fileline(), conRhsp, nodep->lsbConst(), nodep->widthConst()}; - nodep->replaceWith(newp); + nodep->replaceWithKeepDType(newp); } else { // Yuk, split between the two conRhsp->unlinkFrBack(); @@ -2592,7 +2592,7 @@ class ConstVisitor final : public VNVisitor { nodep->msbConst() - conRhsp->width() + 1}, new AstSel{nodep->fileline(), conRhsp, nodep->lsbConst(), conRhsp->width() - nodep->lsbConst()}}; - nodep->replaceWith(newp); + nodep->replaceWithKeepDType(newp); } VL_DO_DANGLING(pushDeletep(nodep), nodep); } @@ -2718,7 +2718,7 @@ class ConstVisitor final : public VNVisitor { nodep->v3error("Illegal assignment of constant to unpacked array"); } else { AstNode* const fromp = nodep->fromp()->unlinkFrBack(); - nodep->replaceWith(fromp); + nodep->replaceWithKeepDType(fromp); if (VN_IS(fromp->dtypep()->skipRefp(), NodeArrayDType)) { // Strip off array to find what array references fromp->dtypeFrom( @@ -2771,12 +2771,12 @@ class ConstVisitor final : public VNVisitor { // This exception is fairly fragile, i.e. doesn't // support arrays of arrays or other stuff AstNode* const newp = valuep->cloneTree(false); - nodep->replaceWith(newp); + nodep->replaceWithKeepDType(newp); VL_DO_DANGLING(pushDeletep(nodep), nodep); did = true; } else if (nodep->varp()->isParam() && VN_IS(valuep, Unbounded)) { AstNode* const newp = valuep->cloneTree(false); - nodep->replaceWith(newp); + nodep->replaceWithKeepDType(newp); VL_DO_DANGLING(pushDeletep(nodep), nodep); did = true; } diff --git a/src/V3Expand.cpp b/src/V3Expand.cpp index 395ba5e04..401923ef3 100644 --- a/src/V3Expand.cpp +++ b/src/V3Expand.cpp @@ -445,7 +445,6 @@ class ExpandVisitor final : public VNVisitor { midp = new AstCond{ nfl, // lsb % VL_EDATASIZE == 0 ? - new AstEq{nfl, new AstConst{nfl, 0}, newSelBitBit(nodep->lsbp())}, // 0 : new AstConst{nfl, zero}, diff --git a/test_regress/t/t_math_cv_bitop.out b/test_regress/t/t_math_cv_bitop.out new file mode 100644 index 000000000..d4536207b --- /dev/null +++ b/test_regress/t/t_math_cv_bitop.out @@ -0,0 +1,6 @@ +one 'd=-1 'b=1 +ort 'd=-1 'b=1 +tmp 'd= -1 'b=11111111 +out63 'd= 1 'b=000000000000000000000001 +out63 'd= 1 'b=000000000000000000000001 +*-* All Finished *-* diff --git a/test_regress/t/t_math_cv_bitop.py b/test_regress/t/t_math_cv_bitop.py new file mode 100755 index 000000000..dcb1ff476 --- /dev/null +++ b/test_regress/t/t_math_cv_bitop.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 by Wilson Snyder. 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 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile(verilator_flags2=["--binary"]) + +test.execute(expect_filename=test.golden_filename) + +test.passes() diff --git a/test_regress/t/t_math_cv_bitop.v b/test_regress/t/t_math_cv_bitop.v new file mode 100644 index 000000000..9a4cbcf6a --- /dev/null +++ b/test_regress/t/t_math_cv_bitop.v @@ -0,0 +1,92 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +`define stop $stop +`define checks(gotv,expv) do if ((gotv) != (expv)) begin $write("%%Error: %s:%0d: got='%s' exp='%s'\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); + +module sub ( + input wire clock_4, + input wire clock_8, + output wire [28:5] out63 +); + + reg [28:0] reg_12; + reg [28:22] reg_24; + + wire _0558_ = | reg_24[26:25]; // reg_24 = 0 or 1100110 ---> _0558_ == 0 + wire [28:0] _0670_ = _0558_ ? reg_12 : 29'h00000f93; // _0558_ == 0 ---> _0670_ == 29'h00000f93 + wire [28:0] _0399_= - _0670_; // _0670_ == 29'h00000f93 ---> _0399_ = 29'b11111111111111111000001101101 + wire _0085_ = ~ _0399_[2]; // _0399_[2] == 1 ---> _0085_ == 0 + wire [28:0] _0769_; + assign { _0769_[28:3], _0769_[1:0] } = { _0399_[28:3], _0399_[1:0] }; // _0769_ != 0 + assign _0769_[2] = _0085_; + + // verilator lint_off WIDTH + wire _0305_ = ! _0769_; // _0769_ != 0 ---> _0305_ == 0 + wire [23:0] _0306_ = ! _0305_; // _0305_ == 0 ---> _0306_ == 1 + // verilator lint_on WIDTH + + assign out63 = _0306_; // out63 == 1 + + always @(posedge clock_4, posedge clock_8) + if (clock_8) reg_12 <= 29'h00000066; + else reg_12 <= { reg_12[28:27], 25'h0000001, reg_12[1:0] }; + + always @(posedge clock_4, posedge clock_8) + if (clock_8) reg_24 <= 7'h66; + else reg_24 <= reg_24; + +endmodule + +module t; + reg clock_4; + reg clock_8; + wire signed [28:5] out63; + reg signed [7:0] tmp = -1; + reg signed [0:0] one = 1; + reg signed [0:0] onert = 1; + + sub sub ( + .clock_4 (clock_4), + .clock_8 (clock_8), + .out63 (out63) + ); + + + initial begin + // All simulators agree: 1'sb1 really shows as decimal -1 + $display("one 'd=%d 'b=%b", one, one); +`ifdef VERILATOR + onert = $c(1); +`endif + $display("ort 'd=%d 'b=%b", onert, onert); + + $display("tmp 'd=%d 'b=%b", tmp, tmp); + + clock_4 = 0; + clock_8 = 0; + #2000; + + sub.reg_24 = 0; + sub.reg_12 = 0; + #2000; + + clock_4 = 0; + clock_8 = 0; + #10; + $display("out63 'd=%d 'b=%b", out63, out63); + + #2000; + clock_4 = 1; + clock_8 = 1; + #10; + $display("out63 'd=%d 'b=%b", out63, out63); + + $write("*-* All Finished *-*\n"); + $finish; + end + +endmodule