VeniVidiVici
/
verilator
mirror of https://github.com/verilator/verilator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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

This commit is contained in:
Wilson Snyder 2025-01-08 19:37:20 -05:00
parent ff244c1d1f
commit 44f49669a3
9 changed files with 172 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Changes
View File

@ -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]

18
src/V3Ast.cpp
View File

@ -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

1
src/V3Ast.h
View File

@ -2077,6 +2077,7 @@ private:
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);

70
src/V3LinkInc.cpp
View File

@ -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,12 +212,69 @@ 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 {
prepost_expr_visit(nodep);
// 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 {
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);
AstConst* const constp = VN_AS(nodep->lhsp(), Const);

12
src/verilog.y
View File

@ -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

18
test_regress/t/t_array_index_side.py Executable file
View File

@ -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()

54
test_regress/t/t_array_index_side.v Normal file
View File

@ -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

14
test_regress/t/t_stmt_incr_unsup.out
View File

@ -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

4
test_regress/t/t_stmt_incr_unsup.v
View File

@ -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