Support side effects of form variable[index_function()]++.

chipsalliance · Jan 9, 2025 · 44f4966 · 44f4966
1 parent ff244c1
commit 44f4966
Show file tree

Hide file tree

Showing 9 changed files with 172 additions and 20 deletions.
diff --git a/Changes b/Changes
@@ -16,6 +16,7 @@ Verilator 5.033 devel
 * Add COVERIGN warning, as a more specific UNSUPPORTED error.
 * Support generated classes (#5665). [Shou-Li Hsu]
 * Support `+incdir` with multiple directories.
+* Support side effects of form 'variable[index_function()]++'.
 * Fix error message when call task as a function (#3089). [Matthew Ballance]
 * Fix V3Simulate constant reuse (#5709). [Geza Lore]
 * Fix man pages what-is section (#5710). [Ahmed El-Mahmoudy]

diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp
@@ -797,13 +797,7 @@ void AstNode::swapWith(AstNode* bp) {
 
 AstNode* AstNode::cloneTreeIter(bool needPure) {
     // private: Clone single node and children
-    if (VL_UNLIKELY(needPure && !isPure())) {
-        this->v3warn(SIDEEFFECT,
-                     "Expression side effect may be mishandled\n"
-                         << this->warnMore()
-                         << "... Suggest use a temporary variable in place of this expression");
-        // this->v3fatalSrc("cloneTreePure debug backtrace");  // Comment in to debug where caused
-    }
+    if (needPure) purityCheck();
     AstNode* const newp = this->clone();
     if (this->m_op1p) newp->op1p(this->m_op1p->cloneTreeIterList(needPure));
     if (this->m_op2p) newp->op2p(this->m_op2p->cloneTreeIterList(needPure));
@@ -850,6 +844,16 @@ AstNode* AstNode::cloneTree(bool cloneNextLink, bool needPure) {
     return newp;
 }
 
+void AstNode::purityCheck() {
+    if (VL_UNLIKELY(!isPure())) {
+        this->v3warn(SIDEEFFECT,
+                     "Expression side effect may be mishandled\n"
+                         << this->warnMore()
+                         << "... Suggest use a temporary variable in place of this expression");
+        // this->v3fatalSrc("cloneTreePure debug backtrace");  // Comment in to debug where caused
+    }
+}
+
 //======================================================================
 // Delete
 

diff --git a/src/V3Ast.h b/src/V3Ast.h
@@ -2077,6 +2077,7 @@ class AstNode VL_NOT_FINAL {
     string instanceStr() const;
 
 public:
+    void purityCheck();
     static void relinkOneLink(AstNode*& pointpr, AstNode* newp);
     // cppcheck-suppress functionConst
     static void debugTreeChange(const AstNode* nodep, const char* prefix, int lineno, bool next);

diff --git a/src/V3LinkInc.cpp b/src/V3LinkInc.cpp
@@ -27,13 +27,20 @@
 //          Create a temporary __VIncrementX variable, assign the value of
 //          of the current variable (after the operation) to it. Substitute
 //          The original variable with the temporary one in the statement.
+//
 //      prepost_stmt_visit
 //        PREADD/PRESUB/POSTADD/POSTSUB
 //          Increment/decrement the current variable by the given value.
 //          The order (pre/post) doesn't matter outside statements thus
 //          the pre/post operations are treated equally and there is no
 //          need for a temporary variable.
 //
+//      prepost_stmt_sel_visit
+//        For e.g. 'array[something_with_side_eff]++', common in UVM etc
+//        PREADD/PRESUB/POSTADD/POSTSUB
+//          Create temporary with array index.
+//          Increment/decrement using index of the temporary.
+//
 //*************************************************************************
 
 #include "V3PchAstNoMT.h"  // VL_MT_DISABLED_CODE_UNIT
@@ -205,11 +212,68 @@ class LinkIncVisitor final : public VNVisitor {
     void visit(AstPropSpec* nodep) override { unsupported_visit(nodep); }
     void prepost_visit(AstNodeTriop* nodep) {
         // Check if we are underneath a statement
-        if (!m_insStmtp) {
-            prepost_stmt_visit(nodep);
+        AstSelBit* const selbitp = VN_CAST(nodep->thsp(), SelBit);
+        if (!m_insStmtp && selbitp && VN_IS(selbitp->fromp(), NodeVarRef)
+            && !selbitp->bitp()->isPure()) {
+            prepost_stmt_sel_visit(nodep);
+        } else {
+            // Purity check was deferred at creation in verilog.y, check now
+            nodep->thsp()->purityCheck();
+            if (!m_insStmtp) {
+                prepost_stmt_visit(nodep);
+            } else {
+                prepost_expr_visit(nodep);
+            }
+        }
+    }
+    void prepost_stmt_sel_visit(AstNodeTriop* nodep) {
+        // Special case array[something]++, see comments at file top
+        // if (debug() >= 9) nodep->dumpTree("-pp-stmt-sel-in:  ");
+        iterateChildren(nodep);
+        AstConst* const constp = VN_AS(nodep->lhsp(), Const);
+        UASSERT_OBJ(nodep, constp, "Expecting CONST");
+        AstConst* const newconstp = constp->cloneTree(true);
+
+        AstSelBit* const rdSelbitp = VN_CAST(nodep->rhsp(), SelBit);
+        AstNodeExpr* const rdFromp = rdSelbitp->fromp()->unlinkFrBack();
+        AstNodeExpr* const rdBitp = rdSelbitp->bitp()->unlinkFrBack();
+        AstSelBit* const wrSelbitp = VN_CAST(nodep->thsp(), SelBit);
+        AstNodeExpr* const wrFromp = wrSelbitp->fromp()->unlinkFrBack();
+
+        // Prepare a temporary variable
+        FileLine* const fl = nodep->fileline();
+        const string name = "__VincIndex"s + cvtToStr(++m_modIncrementsNum);
+        AstVar* const varp = new AstVar{
+            fl, VVarType::BLOCKTEMP, name, VFlagChildDType{},
+            new AstRefDType{fl, AstRefDType::FlagTypeOfExpr{}, rdBitp->cloneTree(true)}};
+        if (m_ftaskp) varp->funcLocal(true);
+
+        // Declare the variable
+        insertOnTop(varp);
+
+        // Define what operation will we be doing
+        AstAssign* const varAssignp
+            = new AstAssign{fl, new AstVarRef{fl, varp, VAccess::WRITE}, rdBitp};
+        AstNode* const newp = varAssignp;
+
+        AstNodeExpr* const valuep
+            = new AstSelBit{fl, rdFromp, new AstVarRef{fl, varp, VAccess::READ}};
+        AstNodeExpr* const storeTop
+            = new AstSelBit{fl, wrFromp, new AstVarRef{fl, varp, VAccess::READ}};
+
+        AstAssign* assignp;
+        if (VN_IS(nodep, PreSub) || VN_IS(nodep, PostSub)) {
+            assignp = new AstAssign{nodep->fileline(), storeTop,
+                                    new AstSub{nodep->fileline(), valuep, newconstp}};
         } else {
-            prepost_expr_visit(nodep);
+            assignp = new AstAssign{nodep->fileline(), storeTop,
+                                    new AstAdd{nodep->fileline(), valuep, newconstp}};
         }
+        newp->addNext(assignp);
+
+        // if (debug() >= 9) newp->dumpTreeAndNext("-pp-stmt-sel-new: ");
+        nodep->replaceWith(newp);
+        VL_DO_DANGLING(nodep->deleteTree(), nodep);
     }
     void prepost_stmt_visit(AstNodeTriop* nodep) {
         iterateChildren(nodep);

diff --git a/src/verilog.y b/src/verilog.y
@@ -3832,17 +3832,21 @@ inc_or_dec_expression<nodeExprp>:   // ==IEEE: inc_or_dec_expression
         //                      // Need fexprScope instead of variable_lvalue to prevent conflict
                 ~l~exprScope yP_PLUSPLUS
                         { $<fl>$ = $<fl>1; $$ = new AstPostAdd{$2, new AstConst{$2, AstConst::StringToParse{}, "'b1"},
-                                                               $1, $1->cloneTreePure(true)}; }
+                                                               // Purity checked in V3LinkInc
+                                                               $1, $1->cloneTree(true)}; }
         |       ~l~exprScope yP_MINUSMINUS
                         { $<fl>$ = $<fl>1; $$ = new AstPostSub{$2, new AstConst{$2, AstConst::StringToParse{}, "'b1"},
-                                                               $1, $1->cloneTreePure(true)}; }
+                                                               // Purity checked in V3LinkInc
+                                                               $1, $1->cloneTree(true)}; }
         //                      // Need expr instead of variable_lvalue to prevent conflict
         |       yP_PLUSPLUS     expr
                         { $<fl>$ = $<fl>1; $$ = new AstPreAdd{$1, new AstConst{$1, AstConst::StringToParse{}, "'b1"},
-                                                              $2, $2->cloneTreePure(true)}; }
+                                                              // Purity checked in V3LinkInc
+                                                              $2, $2->cloneTree(true)}; }
         |       yP_MINUSMINUS   expr
                         { $<fl>$ = $<fl>1; $$ = new AstPreSub{$1, new AstConst{$1, AstConst::StringToParse{}, "'b1"},
-                                                              $2, $2->cloneTreePure(true)}; }
+                                                              // Purity checked in V3LinkInc
+                                                              $2, $2->cloneTree(true)}; }
         ;
 
 finc_or_dec_expression<nodeExprp>:  // ==IEEE: inc_or_dec_expression

diff --git a/test_regress/t/t_array_index_side.py b/test_regress/t/t_array_index_side.py
@@ -0,0 +1,18 @@
+#!/usr/bin/env python3
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2024 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_st')
+
+test.compile()
+
+test.execute()
+
+test.passes()
diff --git a/test_regress/t/t_array_index_side.v b/test_regress/t/t_array_index_side.v
@@ -0,0 +1,54 @@
+// 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 checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d:  got=%0d exp=%0d (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0);
+
+class Cls;
+   int m_index;
+
+   function automatic int get_index();
+      int rtn;
+      rtn = m_index;
+      ++m_index;
+`ifdef VERILATOR
+      return $c(rtn);  // Avoid optimizations
+`else
+      return rtn;
+`endif
+   endfunction
+endclass
+
+module t (/*AUTOARG*/);
+
+   Cls cls;
+   int array[10];
+
+   initial begin
+      cls = new;
+      // Common UVM construct 'id_cnt[get_id()]++;'
+      // Properly avoid/handle SIDEEFF warnings
+      cls.m_index = 5;
+      array[5] = 50;
+      array[6] = 60;
+      array[7] = 70;
+      array[8] = 80;
+
+      array[cls.get_index()]++;
+      `checkd(array[5], 51);
+      array[cls.get_index()]++;
+      `checkd(array[6], 61);
+
+      ++array[cls.get_index()];
+      `checkd(array[7], 71);
+      ++array[cls.get_index()];
+      `checkd(array[8], 81);
+
+      $write("*-* All Finished *-*\n");
+      $finish;
+   end
+
+endmodule
diff --git a/test_regress/t/t_stmt_incr_unsup.out b/test_regress/t/t_stmt_incr_unsup.out
@@ -1,11 +1,17 @@
-%Warning-SIDEEFFECT: t/t_stmt_incr_unsup.v:17:12: Expression side effect may be mishandled
+%Warning-SIDEEFFECT: t/t_stmt_incr_unsup.v:17:31: Expression side effect may be mishandled
                                                 : ... Suggest use a temporary variable in place of this expression
-   17 |         arr[postincrement_i()]++;
-      |            ^
+   17 |         arr[postincrement_i()][postincrement_i()]++;
+      |                               ^
                      ... 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()]++;
+   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
diff --git a/test_regress/t/t_stmt_incr_unsup.v b/test_regress/t/t_stmt_incr_unsup.v
@@ -12,9 +12,9 @@ endfunction
 
 module t;
     initial begin
-        int arr [1:0] = {0, 0};
+        int arr [3][3] = {{1, 2, 3}, {4, 5, 6}, {7, 8, 9}};
         i = 0;
-        arr[postincrement_i()]++;
+        arr[postincrement_i()][postincrement_i()]++;
         $display("Value: %d", i);
     end
 endmodule