From 44f49669a3a76fddea7c7cbe3b43b2ff471cb235 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Wed, 8 Jan 2025 19:37:20 -0500 Subject: [PATCH] Support side effects of form `variable[index_function()]++`. --- Changes | 1 + src/V3Ast.cpp | 18 ++++--- src/V3Ast.h | 1 + src/V3LinkInc.cpp | 70 ++++++++++++++++++++++++++-- src/verilog.y | 12 +++-- test_regress/t/t_array_index_side.py | 18 +++++++ test_regress/t/t_array_index_side.v | 54 +++++++++++++++++++++ test_regress/t/t_stmt_incr_unsup.out | 14 ++++-- test_regress/t/t_stmt_incr_unsup.v | 4 +- 9 files changed, 172 insertions(+), 20 deletions(-) create mode 100755 test_regress/t/t_array_index_side.py create mode 100644 test_regress/t/t_array_index_side.v diff --git a/Changes b/Changes index 6e2f283fb08..1ab2766723b 100644 --- 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 index c7d926a3e3f..cf1873b6a19 100644 --- 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 index 7d6ddf3dc06..ba483fa949d 100644 --- 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 index 0dd3fc3b45b..a72ff6e1cf8 100644 --- a/src/V3LinkInc.cpp +++ b/src/V3LinkInc.cpp @@ -27,6 +27,7 @@ // 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. @@ -34,6 +35,12 @@ // 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 index 9e292785eb0..8e6a48ea768 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -3832,17 +3832,21 @@ inc_or_dec_expression: // ==IEEE: inc_or_dec_expression // // Need fexprScope instead of variable_lvalue to prevent conflict ~l~exprScope yP_PLUSPLUS { $$ = $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 { $$ = $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 { $$ = $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 { $$ = $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: // ==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 new file mode 100755 index 00000000000..dbdaf45516d --- /dev/null +++ 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 new file mode 100644 index 00000000000..679edbe7040 --- /dev/null +++ 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 index c7c313981a8..a7ac922914e 100644 --- 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 index e1682331a59..09e258028d4 100644 --- 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