From 83f4db956b2be8f79d57e47640ef1cea1186ac00 Mon Sep 17 00:00:00 2001 From: Igor Zaworski Date: Mon, 29 Sep 2025 14:20:54 +0200 Subject: [PATCH] Fix side effects when using select (#6460) --- src/V3Unknown.cpp | 99 ++++++++++++++++++------ test_regress/t/t_lint_sideeffect_bad.out | 9 +-- test_regress/t/t_select_sideeffect.py | 18 +++++ test_regress/t/t_select_sideeffect.v | 34 ++++++++ test_regress/t/t_stmt_incr_unsup.out | 10 --- 5 files changed, 130 insertions(+), 40 deletions(-) create mode 100755 test_regress/t/t_select_sideeffect.py create mode 100644 test_regress/t/t_select_sideeffect.v diff --git a/src/V3Unknown.cpp b/src/V3Unknown.cpp index 4086bed42..bca507440 100644 --- a/src/V3Unknown.cpp +++ b/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); - if (condp->isOne()) { + AstConst* const maxmsbConstp = new AstConst{ + 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); - } else if (!lvalue) { + VL_DO_DANGLING(maxmsbConstp->deleteTree(), maxmsbConstp); + 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 - = new AstGte{nodep->fileline(), - new AstConst(nodep->fileline(), AstConst::WidthedValue{}, - nodep->bitp()->width(), declElements - 1), - nodep->bitp()->cloneTreePure(false)}; - // 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()) { + AstConst* const declElementsp + = new AstConst{nodep->fileline(), AstConst::WidthedValue{}, nodep->bitp()->width(), + static_cast(declElements - 1)}; + AstNodeExpr* bitp = V3Const::constifyEdit(nodep->bitp()->unlinkFrBack()); + if (isStaticlyGte(declElementsp, bitp)) { // We don't need to add a conditional; we know the existing expression is ok - VL_DO_DANGLING(condp->deleteTree(), condp); - } else if (!lvalue - // Making a scalar would break if we're making an array - && VN_IS(nodeDtp, BasicDType)) { + VL_DO_DANGLING(declElementsp->deleteTree(), declElementsp); + nodep->bitp(bitp); + return; + } + 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(bitunlinkFrBack(&replaceHandle); diff --git a/test_regress/t/t_lint_sideeffect_bad.out b/test_regress/t/t_lint_sideeffect_bad.out index 7b2dbd3f4..59695b538 100644 --- a/test_regress/t/t_lint_sideeffect_bad.out +++ b/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; diff --git a/test_regress/t/t_select_sideeffect.py b/test_regress/t/t_select_sideeffect.py new file mode 100755 index 000000000..f989a35fb --- /dev/null +++ b/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() diff --git a/test_regress/t/t_select_sideeffect.v b/test_regress/t/t_select_sideeffect.v new file mode 100644 index 000000000..37a57ae3a --- /dev/null +++ b/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 diff --git a/test_regress/t/t_stmt_incr_unsup.out b/test_regress/t/t_stmt_incr_unsup.out index a7ac92291..cd390f877 100644 --- a/test_regress/t/t_stmt_incr_unsup.out +++ b/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