Fix side effects when using select (#6460)

src/V3Unknown.cpp

@@ -57,6 +57,7 @@ class UnknownVisitor final : public VNVisitor {
 
     // STATE - for current visit position (use VL_RESTORER)
     AstNodeModule* m_modp = nullptr;  // Current module
+    AstNodeFTask* m_ftaskp = nullptr;  // Current function/task
     AstAssignW* m_assignwp = nullptr;  // Current assignment
     AstAssignDly* m_assigndlyp = nullptr;  // Current assignment
     AstNode* m_timingControlp = nullptr;  // Current assignment's intra timing control
@@ -140,6 +141,46 @@ class UnknownVisitor final : public VNVisitor {
         }
     }
 
+    AstVar* createAddTemp(const AstNodeExpr* const nodep) {
+        AstVar* const varp = new AstVar{nodep->fileline(), VVarType::XTEMP,
+                                        m_xrandNames->get(nodep), nodep->dtypep()};
+        if (m_ftaskp) {
+            varp->funcLocal(true);
+            varp->lifetime(VLifetime::AUTOMATIC_EXPLICIT);
+            m_ftaskp->stmtsp()->addHereThisAsNext(varp);
+        } else {
+            m_modp->stmtsp()->addHereThisAsNext(varp);
+        }
+        return varp;
+    }
+
+    // Returns true if it is known at compile time that `msbConstp` is greater than or equal
+    // `exprp`
+    static bool isStaticlyGte(AstConst* const msbConstp, const AstNodeExpr* const exprp) {
+        if (msbConstp->width() >= exprp->width()
+            && msbConstp->num().toSInt() >= (1 << exprp->width()) - 1) {
+            return true;
+        }
+        if (const AstConst* const constp = VN_CAST(exprp, Const)) {
+            if (V3Number{msbConstp}.opGte(msbConstp->num(), constp->num()).isNeqZero()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    AstNodeExpr* newExprStmtOrClone(AstNodeExpr*& exprp) {
+        if (!exprp->isPure()) {
+            AstVar* const varp = createAddTemp(exprp);
+            FileLine* const fl = exprp->fileline();
+            exprp = new AstExprStmt{
+                fl, new AstAssign{fl, new AstVarRef{fl, varp, VAccess::WRITE}, exprp},
+                new AstVarRef{fl, varp, VAccess::READ}};
+            return new AstVarRef{fl, varp, VAccess::READ};
+        }
+        return exprp->cloneTreePure(false);
+    }
+
     // VISITORS
     void visit(AstNodeModule* nodep) override {
         UINFO(4, " MOD   " << nodep);
@@ -158,6 +199,11 @@ class UnknownVisitor final : public VNVisitor {
             xrandNames.swap(m_xrandNames);
         }
     }
+    void visit(AstNodeFTask* nodep) override {
+        VL_RESTORER(m_ftaskp);
+        m_ftaskp = nodep;
+        iterateChildren(nodep);
+    }
     void visit(AstAssignDly* nodep) override {
         VL_RESTORER(m_assigndlyp);
         VL_RESTORER(m_timingControlp);
@@ -387,22 +433,25 @@ class UnknownVisitor final : public VNVisitor {
             }
             // Find range of dtype we are selecting from
             // Similar code in V3Const::warnSelect
-            const int maxmsb = nodep->fromp()->dtypep()->width() - 1;
+            const uint32_t maxmsb = nodep->fromp()->dtypep()->width() - 1;
             UINFOTREE(9, nodep, "", "sel_old");
 
             // If (maxmsb >= selected), we're in bound
-            AstNodeExpr* condp
-                = new AstGte{nodep->fileline(),
-                             new AstConst(nodep->fileline(), AstConst::WidthedValue{},
-                                          nodep->lsbp()->width(), maxmsb),
-                             nodep->lsbp()->cloneTreePure(false)};
             // See if the condition is constant true (e.g. always in bound due to constant select)
             // Note below has null backp(); the Edit function knows how to deal with that.
-            condp = V3Const::constifyEdit(condp);
+            AstConst* const maxmsbConstp = new AstConst{
-            if (condp->isOne()) {
+                nodep->fileline(), AstConst::WidthedValue{}, nodep->lsbp()->width(), maxmsb};
+            AstNodeExpr* lsbp = V3Const::constifyEdit(nodep->lsbp()->unlinkFrBack());
+            if (isStaticlyGte(maxmsbConstp, lsbp)) {
                 // We don't need to add a conditional; we know the existing expression is ok
-                VL_DO_DANGLING(condp->deleteTree(), condp);
+                VL_DO_DANGLING(maxmsbConstp->deleteTree(), maxmsbConstp);
-            } else if (!lvalue) {
+                nodep->lsbp(lsbp);
+                return;
+            }
+            nodep->lsbp(newExprStmtOrClone(lsbp));
+            AstNodeExpr* condp
+                = V3Const::constifyEdit(new AstGte{nodep->fileline(), maxmsbConstp, lsbp});
+            if (!lvalue) {
                 // SEL(...) -> COND(LTE(bit<=maxmsb), ARRAYSEL(...), {width{1'bx}})
                 VNRelinker replaceHandle;
                 nodep->unlinkFrBack(&replaceHandle);
@@ -466,21 +515,25 @@ class UnknownVisitor final : public VNVisitor {
                     }
                 }
             }
+
             // See if the condition is constant true
-            AstNodeExpr* condp
+            AstConst* const declElementsp
-                = new AstGte{nodep->fileline(),
+                = new AstConst{nodep->fileline(), AstConst::WidthedValue{}, nodep->bitp()->width(),
-                             new AstConst(nodep->fileline(), AstConst::WidthedValue{},
+                               static_cast<uint32_t>(declElements - 1)};
-                                          nodep->bitp()->width(), declElements - 1),
+            AstNodeExpr* bitp = V3Const::constifyEdit(nodep->bitp()->unlinkFrBack());
-                             nodep->bitp()->cloneTreePure(false)};
+            if (isStaticlyGte(declElementsp, bitp)) {
-            // Note below has null backp(); the Edit function knows how to deal with that.
-            condp = V3Const::constifyEdit(condp);
-            const AstNodeDType* const nodeDtp = nodep->dtypep()->skipRefp();
-            if (condp->isOne()) {
                 // We don't need to add a conditional; we know the existing expression is ok
-                VL_DO_DANGLING(condp->deleteTree(), condp);
+                VL_DO_DANGLING(declElementsp->deleteTree(), declElementsp);
-            } else if (!lvalue
+                nodep->bitp(bitp);
-                       // Making a scalar would break if we're making an array
+                return;
-                       && VN_IS(nodeDtp, BasicDType)) {
+            }
+            nodep->bitp(newExprStmtOrClone(bitp));
+            AstNodeExpr* condp = new AstGte{nodep->fileline(), declElementsp, bitp};
+            // Note below has null backp(); the Edit function knows how to deal with that.
+            const AstNodeDType* const nodeDtp = nodep->dtypep()->skipRefp();
+            if (!lvalue
+                // Making a scalar would break if we're making an array
+                && VN_IS(nodeDtp, BasicDType)) {
                 // ARRAYSEL(...) -> COND(LT(bit<maxbit), ARRAYSEL(...), {width{1'bx}})
                 VNRelinker replaceHandle;
                 nodep->unlinkFrBack(&replaceHandle);

test_regress/t/t_lint_sideeffect_bad.out

@@ -1,14 +1,9 @@
-%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:19:31: Expression side effect may be mishandled
-                                                    : ... note: In instance 't'
-                                                    : ... Suggest use a temporary variable in place of this expression
-   19 |       $display("0x%8x", array[$c(0) +: 32]);
-      |                               ^~
-                     ... For warning description see https://verilator.org/warn/SIDEEFFECT?v=latest
-                     ... Use "/* verilator lint_off SIDEEFFECT */" and lint_on around source to disable this message.
 %Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:12:13: Expression side effect may be mishandled
                                                     : ... Suggest use a temporary variable in place of this expression
    12 |       case ($c("1"))
       |             ^~
+                     ... For warning description see https://verilator.org/warn/SIDEEFFECT?v=latest
+                     ... Use "/* verilator lint_off SIDEEFFECT */" and lint_on around source to disable this message.
 %Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:13:10: Expression side effect may be mishandled
                                                     : ... Suggest use a temporary variable in place of this expression
    13 |         1: $stop;

test_regress/t/t_select_sideeffect.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()
+
+test.execute()
+
+test.passes()

test_regress/t/t_select_sideeffect.v

@@ -0,0 +1,34 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2025 by Antmicro.
+// SPDX-License-Identifier: CC0-1.0
+
+class Foo;
+  bit [2:0] x = 0;
+  function int get();
+    x += 1;
+    return int'(x);
+  endfunction
+  function bit [2:0] get2();
+    x += 1;
+    return x;
+  endfunction
+endclass
+
+module t;
+  Foo foo;
+  int x[5] = {1, 2, 3, 4, 5};
+  initial begin
+    foo = new;
+    if (x[foo.get()] != 2) $stop;
+    $write("*-* All Finished *-*\n");
+    $finish;
+  end
+  always begin
+    if (x[foo.get2()] != 3) $stop;
+  end
+  final begin
+    if (x[foo.get()] != 4) $stop;
+  end
+endmodule

test_regress/t/t_stmt_incr_unsup.out

@@ -4,14 +4,4 @@
       |                               ^
                      ... For warning description see https://verilator.org/warn/SIDEEFFECT?v=latest
                      ... Use "/* verilator lint_off SIDEEFFECT */" and lint_on around source to disable this message.
-%Warning-SIDEEFFECT: t/t_stmt_incr_unsup.v:17:13: Expression side effect may be mishandled
-                                                : ... note: In instance 't'
-                                                : ... Suggest use a temporary variable in place of this expression
-   17 |         arr[postincrement_i()][postincrement_i()]++;
-      |             ^~~~~~~~~~~~~~~
-%Warning-SIDEEFFECT: t/t_stmt_incr_unsup.v:17:32: Expression side effect may be mishandled
-                                                : ... note: In instance 't'
-                                                : ... Suggest use a temporary variable in place of this expression
-   17 |         arr[postincrement_i()][postincrement_i()]++;
-      |                                ^~~~~~~~~~~~~~~
 %Error: Exiting due to