Fix shift to remove operation side effects (#4563).

Changes

@@ -38,6 +38,7 @@ Verilator 5.017 devel
 * Fix inlining of real functions miscasting (#4543). [Andrew Nolte]
 * Fix broken link error for enum references (#4551). [Anthony Donlon]
 * Fix instance arrays connecting to array of structs (#4557). [raphmaster]
+* Fix shift to remove operation side effects (#4563).
 * Fix preprocessor to show `line 2 on resumed file.
 
 

include/verilated_funcs.h

@@ -1647,10 +1647,20 @@ static inline void _vl_shiftl_inplace_w(int obits, WDataOutP iowp,
 // EMIT_RULE: VL_SHIFTL:  oclean=lclean; rclean==clean;
 // Important: Unlike most other funcs, the shift might well be a computed
 // expression.  Thus consider this when optimizing.  (And perhaps have 2 funcs?)
+// If RHS (rd/rwp) is larger than the output, zeros (or all ones for >>>) must be returned
+// (This corresponds to AstShift*Ovr Ast nodes)
+static inline IData VL_SHIFTL_III(int obits, int, int, IData lhs, IData rhs) VL_MT_SAFE {
+    if (VL_UNLIKELY(rhs >= VL_IDATASIZE)) return 0;
+    return lhs << rhs;  // Small is common so not clean return
+}
 static inline IData VL_SHIFTL_IIQ(int obits, int, int, IData lhs, QData rhs) VL_MT_SAFE {
     if (VL_UNLIKELY(rhs >= VL_IDATASIZE)) return 0;
     return VL_CLEAN_II(obits, obits, lhs << rhs);
 }
+static inline QData VL_SHIFTL_QQI(int obits, int, int, QData lhs, IData rhs) VL_MT_SAFE {
+    if (VL_UNLIKELY(rhs >= VL_QUADSIZE)) return 0;
+    return lhs << rhs;  // Small is common so not clean return
+}
 static inline QData VL_SHIFTL_QQQ(int obits, int, int, QData lhs, QData rhs) VL_MT_SAFE {
     if (VL_UNLIKELY(rhs >= VL_QUADSIZE)) return 0;
     return VL_CLEAN_QQ(obits, obits, lhs << rhs);
@@ -1692,7 +1702,7 @@ static inline IData VL_SHIFTL_IIW(int obits, int, int rbits, IData lhs,
             return 0;
         }
     }
-    return VL_CLEAN_II(obits, obits, lhs << rwp[0]);
+    return VL_SHIFTL_III(obits, obits, 32, lhs, rwp[0]);
 }
 static inline QData VL_SHIFTL_QQW(int obits, int, int rbits, QData lhs,
                                   WDataInP const rwp) VL_MT_SAFE {
@@ -1702,16 +1712,24 @@ static inline QData VL_SHIFTL_QQW(int obits, int, int rbits, QData lhs,
         }
     }
     // Above checks rwp[1]==0 so not needed in below shift
-    return VL_CLEAN_QQ(obits, obits, lhs << (static_cast<QData>(rwp[0])));
+    return VL_SHIFTL_QQI(obits, obits, 32, lhs, rwp[0]);
 }
 
 // EMIT_RULE: VL_SHIFTR:  oclean=lclean; rclean==clean;
 // Important: Unlike most other funcs, the shift might well be a computed
 // expression.  Thus consider this when optimizing.  (And perhaps have 2 funcs?)
+static inline IData VL_SHIFTR_III(int obits, int, int, IData lhs, IData rhs) VL_PURE {
+    if (VL_UNLIKELY(rhs >= VL_IDATASIZE)) return 0;
+    return lhs >> rhs;  // Small is common so assumed not clean
+}
 static inline IData VL_SHIFTR_IIQ(int obits, int, int, IData lhs, QData rhs) VL_PURE {
     if (VL_UNLIKELY(rhs >= VL_IDATASIZE)) return 0;
     return VL_CLEAN_QQ(obits, obits, lhs >> rhs);
 }
+static inline QData VL_SHIFTR_QQI(int obits, int, int, QData lhs, IData rhs) VL_PURE {
+    if (VL_UNLIKELY(rhs >= VL_QUADSIZE)) return 0;
+    return lhs >> rhs;  // Small is common so assumed not clean
+}
 static inline QData VL_SHIFTR_QQQ(int obits, int, int, QData lhs, QData rhs) VL_PURE {
     if (VL_UNLIKELY(rhs >= VL_QUADSIZE)) return 0;
     return VL_CLEAN_QQ(obits, obits, lhs >> rhs);
@@ -1761,15 +1779,14 @@ static inline IData VL_SHIFTR_IIW(int obits, int, int rbits, IData lhs,
     for (int i = 1; i < VL_WORDS_I(rbits); ++i) {
         if (VL_UNLIKELY(rwp[i])) return 0;  // Huge shift 1>>32 or more
     }
-    return VL_CLEAN_II(obits, obits, lhs >> rwp[0]);
+    return VL_SHIFTR_III(obits, obits, 32, lhs, rwp[0]);
 }
 static inline QData VL_SHIFTR_QQW(int obits, int, int rbits, QData lhs,
                                   WDataInP const rwp) VL_PURE {
     for (int i = 1; i < VL_WORDS_I(rbits); ++i) {
         if (VL_UNLIKELY(rwp[i])) return 0;  // Huge shift 1>>32 or more
     }
-    // Above checks rwp[1]==0 so not needed in below shift
+    return VL_SHIFTR_QQI(obits, obits, 32, lhs, rwp[0]);
-    return VL_CLEAN_QQ(obits, obits, lhs >> (static_cast<QData>(rwp[0])));
 }
 
 // EMIT_RULE: VL_SHIFTRS:  oclean=false; lclean=clean, rclean==clean;
@@ -1779,11 +1796,13 @@ static inline IData VL_SHIFTRS_III(int obits, int lbits, int, IData lhs, IData r
     // must use lbits for sign; lbits might != obits,
     // an EXTEND(SHIFTRS(...)) can became a SHIFTRS(...) within same 32/64 bit word length
     const IData sign = -(lhs >> (lbits - 1));  // ffff_ffff if negative
+    if (VL_UNLIKELY(rhs >= VL_IDATASIZE)) return sign & VL_MASK_I(obits);
     const IData signext = ~(VL_MASK_I(lbits) >> rhs);  // One with bits where we've shifted "past"
     return (lhs >> rhs) | (sign & VL_CLEAN_II(obits, obits, signext));
 }
 static inline QData VL_SHIFTRS_QQI(int obits, int lbits, int, QData lhs, IData rhs) VL_PURE {
     const QData sign = -(lhs >> (lbits - 1));
+    if (VL_UNLIKELY(rhs >= VL_QUADSIZE)) return sign & VL_MASK_Q(obits);
     const QData signext = ~(VL_MASK_Q(lbits) >> rhs);
     return (lhs >> rhs) | (sign & VL_CLEAN_QQ(obits, obits, signext));
 }

src/V3AstNodeExpr.h

@@ -3279,6 +3279,30 @@ public:
     bool sizeMattersLhs() const override { return true; }
     bool sizeMattersRhs() const override { return false; }
 };
+class AstShiftLOvr final : public AstNodeBiop {
+    // Like ShiftL but checks for an over shift and returns zeros
+    // TODO: All Verilog shifts should start as these and later get demoted to AstShiftL
+public:
+    AstShiftLOvr(FileLine* fl, AstNodeExpr* lhsp, AstNodeExpr* rhsp, int setwidth = 0)
+        : ASTGEN_SUPER_ShiftLOvr(fl, lhsp, rhsp) {
+        if (setwidth) dtypeSetLogicSized(setwidth, VSigning::UNSIGNED);
+    }
+    ASTGEN_MEMBERS_AstShiftLOvr;
+    AstNodeExpr* cloneType(AstNodeExpr* lhsp, AstNodeExpr* rhsp) override {
+        return new AstShiftLOvr{fileline(), lhsp, rhsp};
+    }
+    void numberOperate(V3Number& out, const V3Number& lhs, const V3Number& rhs) override {
+        out.opShiftL(lhs, rhs);
+    }
+    string emitVerilog() override { return "%k(%l %f<< %r)"; }
+    string emitC() override { return "VL_SHIFTL_%nq%lq%rq(%nw,%lw,%rw, %P, %li, %ri)"; }
+    string emitSimpleOperator() override { return ""; }
+    bool cleanOut() const override { return false; }
+    bool cleanLhs() const override { return false; }
+    bool cleanRhs() const override { return true; }
+    bool sizeMattersLhs() const override { return true; }
+    bool sizeMattersRhs() const override { return false; }
+};
 class AstShiftR final : public AstNodeBiop {
 public:
     AstShiftR(FileLine* fl, AstNodeExpr* lhsp, AstNodeExpr* rhsp, int setwidth = 0)
@@ -3304,6 +3328,31 @@ public:
     bool sizeMattersLhs() const override { return false; }
     bool sizeMattersRhs() const override { return false; }
 };
+class AstShiftROvr final : public AstNodeBiop {
+    // Like ShiftR but checks for an over shift and returns zeros
+    // TODO: All Verilog shifts should start as these and later get demoted to AstShiftR
+public:
+    AstShiftROvr(FileLine* fl, AstNodeExpr* lhsp, AstNodeExpr* rhsp, int setwidth = 0)
+        : ASTGEN_SUPER_ShiftROvr(fl, lhsp, rhsp) {
+        if (setwidth) dtypeSetLogicSized(setwidth, VSigning::UNSIGNED);
+    }
+    ASTGEN_MEMBERS_AstShiftROvr;
+    AstNodeExpr* cloneType(AstNodeExpr* lhsp, AstNodeExpr* rhsp) override {
+        return new AstShiftROvr{fileline(), lhsp, rhsp};
+    }
+    void numberOperate(V3Number& out, const V3Number& lhs, const V3Number& rhs) override {
+        out.opShiftR(lhs, rhs);
+    }
+    string emitVerilog() override { return "%k(%l %f>> %r)"; }
+    string emitC() override { return "VL_SHIFTR_%nq%lq%rq(%nw,%lw,%rw, %P, %li, %ri)"; }
+    string emitSimpleOperator() override { return ""; }
+    bool cleanOut() const override { return false; }
+    bool cleanLhs() const override { return true; }
+    bool cleanRhs() const override { return true; }
+    // LHS size might be > output size, so don't want to force size
+    bool sizeMattersLhs() const override { return false; }
+    bool sizeMattersRhs() const override { return false; }
+};
 class AstShiftRS final : public AstNodeBiop {
     // Shift right with sign extension, >>> operator
     // Output data type's width determines which bit is used for sign extension
@@ -3330,6 +3379,33 @@ public:
     bool sizeMattersRhs() const override { return false; }
     bool signedFlavor() const override { return true; }
 };
+class AstShiftRSOvr final : public AstNodeBiop {
+    // Shift right with sign extension, >>> operator, checks for an over shift and returns zeros
+    // Output data type's width determines which bit is used for sign extension
+    // TODO: All Verilog shifts should start as these and later get demoted to AstShiftRSOvr
+public:
+    AstShiftRSOvr(FileLine* fl, AstNodeExpr* lhsp, AstNodeExpr* rhsp, int setwidth = 0)
+        : ASTGEN_SUPER_ShiftRSOvr(fl, lhsp, rhsp) {
+        // Important that widthMin be correct, as opExtend requires it after V3Expand
+        if (setwidth) dtypeSetLogicSized(setwidth, VSigning::SIGNED);
+    }
+    ASTGEN_MEMBERS_AstShiftRSOvr;
+    AstNodeExpr* cloneType(AstNodeExpr* lhsp, AstNodeExpr* rhsp) override {
+        return new AstShiftRSOvr{fileline(), lhsp, rhsp};
+    }
+    void numberOperate(V3Number& out, const V3Number& lhs, const V3Number& rhs) override {
+        out.opShiftRS(lhs, rhs, lhsp()->widthMinV());
+    }
+    string emitVerilog() override { return "%k(%l %f>>> %r)"; }
+    string emitC() override { return "VL_SHIFTRS_%nq%lq%rq(%nw,%lw,%rw, %P, %li, %ri)"; }
+    string emitSimpleOperator() override { return ""; }
+    bool cleanOut() const override { return false; }
+    bool cleanLhs() const override { return true; }
+    bool cleanRhs() const override { return true; }
+    bool sizeMattersLhs() const override { return false; }
+    bool sizeMattersRhs() const override { return false; }
+    bool signedFlavor() const override { return true; }
+};
 class AstSub final : public AstNodeBiop {
 public:
     AstSub(FileLine* fl, AstNodeExpr* lhsp, AstNodeExpr* rhsp)

src/V3Const.cpp

@@ -3392,8 +3392,11 @@ private:
     TREEOP ("AstPowSU {$lhsp.isZero, !$rhsp.isZero}",   "replaceZeroChkPure(nodep,$rhsp)");
     TREEOP ("AstOr    {$lhsp.isZero, $rhsp}",   "replaceWRhs(nodep)");
     TREEOP ("AstShiftL    {$lhsp.isZero, $rhsp}",  "replaceZeroChkPure(nodep,$rhsp)");
+    TREEOP ("AstShiftLOvr {$lhsp.isZero, $rhsp}",  "replaceZeroChkPure(nodep,$rhsp)");
     TREEOP ("AstShiftR    {$lhsp.isZero, $rhsp}",  "replaceZeroChkPure(nodep,$rhsp)");
+    TREEOP ("AstShiftROvr {$lhsp.isZero, $rhsp}",  "replaceZeroChkPure(nodep,$rhsp)");
     TREEOP ("AstShiftRS   {$lhsp.isZero, $rhsp}",  "replaceZeroChkPure(nodep,$rhsp)");
+    TREEOP ("AstShiftRSOvr{$lhsp.isZero, $rhsp}",  "replaceZeroChkPure(nodep,$rhsp)");
     TREEOP ("AstXor   {$lhsp.isZero, $rhsp}",   "replaceWRhs(nodep)");
     TREEOP ("AstSub   {$lhsp.isZero, $rhsp}",   "AstNegate{$rhsp}");
     TREEOP ("AstAdd   {$lhsp, $rhsp.isZero}",   "replaceWLhs(nodep)");
@@ -3404,8 +3407,11 @@ private:
     TREEOP ("AstMulS  {$lhsp, $rhsp.isZero}",   "replaceZeroChkPure(nodep,$lhsp)");
     TREEOP ("AstOr    {$lhsp, $rhsp.isZero}",   "replaceWLhs(nodep)");
     TREEOP ("AstShiftL    {$lhsp, $rhsp.isZero}",  "replaceWLhs(nodep)");
+    TREEOP ("AstShiftLOvr {$lhsp, $rhsp.isZero}",  "replaceWLhs(nodep)");
     TREEOP ("AstShiftR    {$lhsp, $rhsp.isZero}",  "replaceWLhs(nodep)");
+    TREEOP ("AstShiftROvr {$lhsp, $rhsp.isZero}",  "replaceWLhs(nodep)");
     TREEOP ("AstShiftRS   {$lhsp, $rhsp.isZero}",  "replaceWLhs(nodep)");
+    TREEOP ("AstShiftRSOvr{$lhsp, $rhsp.isZero}",  "replaceWLhs(nodep)");
     TREEOP ("AstSub   {$lhsp, $rhsp.isZero}",   "replaceWLhs(nodep)");
     TREEOP ("AstXor   {$lhsp, $rhsp.isZero}",   "replaceWLhs(nodep)");
     // Non-zero on one side or the other
@@ -3501,7 +3507,9 @@ private:
     TREEOP ("AstLogNot{$lhsp.castGteS}",                "AstLtS {$lhsp->castGteS()->lhsp(),$lhsp->castGteS()->rhsp()}");
     // Not common, but avoids compiler warnings about over shifting
     TREEOP ("AstShiftL   {operandHugeShiftL(nodep)}",   "replaceZero(nodep)");
+    TREEOP ("AstShiftLOvr{operandHugeShiftL(nodep)}",   "replaceZero(nodep)");
     TREEOP ("AstShiftR   {operandHugeShiftR(nodep)}",   "replaceZero(nodep)");
+    TREEOP ("AstShiftROvr{operandHugeShiftR(nodep)}",   "replaceZero(nodep)");
     TREEOP ("AstShiftL{operandShiftOp(nodep)}",         "replaceShiftOp(nodep)");
     TREEOP ("AstShiftR{operandShiftOp(nodep)}",         "replaceShiftOp(nodep)");
     TREEOP ("AstShiftL{operandShiftShift(nodep)}",      "replaceShiftShift(nodep)");

src/V3Premit.cpp

@@ -49,11 +49,8 @@ class PremitVisitor final : public VNVisitor {
 private:
     // NODE STATE
     //  AstNodeExpr::user()     -> bool.  True if iterated already
-    //  AstShiftL::user2()      -> bool.  True if converted to conditional
-    //  AstShiftR::user2()      -> bool.  True if converted to conditional
     //  *::user3()              -> Used when visiting AstNodeAssign
     const VNUser1InUse m_inuser1;
-    const VNUser2InUse m_inuser2;
 
     // STATE - across all visitors
     VDouble0 m_extractedToConstPool;  // Statistic tracking
@@ -236,7 +233,6 @@ private:
     }
     void visitShift(AstNodeBiop* nodep) {
         // Shifts of > 32/64 bits in C++ will wrap-around and generate non-0s
-        if (!nodep->user2SetOnce()) {
         UINFO(4, "  ShiftFix  " << nodep << endl);
         const AstConst* const shiftp = VN_CAST(nodep->rhsp(), Const);
         if (shiftp && shiftp->num().mostSetBitP1() > 32) {
@@ -248,35 +244,22 @@ private:
                                      // C operator's width must be < maximum shift which is
                                      // based on Verilog width
             && nodep->width() < (1LL << nodep->rhsp()->widthMin())) {
-                VNRelinker replaceHandle;
+            AstNode* newp;
-                nodep->unlinkFrBack(&replaceHandle);
+            if (VN_IS(nodep, ShiftL)) {
-                AstNodeExpr* constzerop;
+                newp = new AstShiftLOvr{nodep->fileline(), nodep->lhsp()->unlinkFrBack(),
-                const int m1value
+                                        nodep->rhsp()->unlinkFrBack()};
-                    = nodep->widthMin() - 1;  // Constant of width-1; not changing dtype width
+            } else if (VN_IS(nodep, ShiftR)) {
-                if (nodep->signedFlavor()) {
+                newp = new AstShiftROvr{nodep->fileline(), nodep->lhsp()->unlinkFrBack(),
-                    // Then over shifting gives the sign bit, not all zeros
+                                        nodep->rhsp()->unlinkFrBack()};
-                    // Note *NOT* clean output -- just like normal shift!
-                    // Create equivalent of VL_SIGNONES_(node_width)
-                    constzerop = new AstNegate{
-                        nodep->fileline(),
-                        new AstShiftR{nodep->fileline(), nodep->lhsp()->cloneTreePure(false),
-                                      new AstConst(nodep->fileline(), m1value), nodep->width()}};
             } else {
-                    constzerop = new AstConst{nodep->fileline(), AstConst::WidthedValue{},
+                UASSERT_OBJ(VN_IS(nodep, ShiftRS), nodep, "Bad case");
-                                              nodep->width(), 0};
+                newp = new AstShiftRSOvr{nodep->fileline(), nodep->lhsp()->unlinkFrBack(),
-                }
+                                         nodep->rhsp()->unlinkFrBack()};
-                constzerop->dtypeFrom(nodep);  // unsigned
-
-                AstNodeExpr* const constwidthp
-                    = new AstConst(nodep->fileline(), AstConst::WidthedValue{},
-                                   nodep->rhsp()->widthMin(), m1value);
-                constwidthp->dtypeFrom(nodep->rhsp());  // unsigned
-                AstCond* const newp = new AstCond{nodep->fileline(),
-                                                  new AstGte{nodep->fileline(), constwidthp,
-                                                             nodep->rhsp()->cloneTreePure(false)},
-                                                  nodep, constzerop};
-                replaceHandle.relink(newp);
             }
+            newp->dtypeFrom(nodep);
+            nodep->replaceWith(newp);
+            VL_DO_DANGLING(pushDeletep(nodep), nodep);
+            return;
         }
         iterateChildren(nodep);
         checkNode(nodep);
@@ -403,9 +386,6 @@ public:
     }
 };
 
-//----------------------------------------------------------------------
-// Top loop
-
 //######################################################################
 // Premit class functions
 

test_regress/t/t_const_opt.pl

@@ -20,7 +20,7 @@ execute(
     );
 
 if ($Self->{vlt}) {
-    file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 39);
+    file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 36);
 }
 ok(1);
 1;

test_regress/t/t_const_opt_no_expand.pl

@@ -21,7 +21,7 @@ execute(
     );
 
 if ($Self->{vlt}) {
-    file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 2);
+    file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 1);
 }
 ok(1);
 1;

test_regress/t/t_math_shift_side.pl

@@ -0,0 +1,21 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2023 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
+
+scenarios(simulator => 1);
+
+compile(
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;

test_regress/t/t_math_shift_side.v

@@ -0,0 +1,40 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Wilson Snyder.
+// SPDX-License-Identifier: CC0-1.0
+
+class Cls;
+   int m_n_bits;
+
+   function int get_n_bytes;
+      return ((m_n_bits - 1) / 8) + 1;
+   endfunction
+
+endclass
+
+module t(/*AUTOARG*/);
+
+   int  i;
+
+   initial begin
+      Cls c;
+      c = new;
+
+      c.m_n_bits = 23;
+      if (c.get_n_bytes() != 3) $stop;
+
+      i = 1 << c.get_n_bytes();
+      if (i != 8) $stop;
+
+      i = 32'h1234 >> c.get_n_bytes();
+      if (i != 32'h246) $stop;
+
+      i = 32'shffffffff >>> c.get_n_bytes();
+      if (i != 32'hffffffff) $stop;
+
+      $write("*-* All Finished *-*\n");
+      $finish;
+   end
+
+endmodule