From 6db599da454ecc491ff6a7d109d239dea1addf5a Mon Sep 17 00:00:00 2001 From: Ryszard Rozak Date: Wed, 14 May 2025 17:55:02 +0200 Subject: [PATCH] Fix slicing of AstExprStmt nodes (#6005) --- src/V3Slice.cpp | 57 +++++++++++++------ src/astgen | 4 +- test_regress/t/t_slice_cond_2d_side_effect.py | 18 ++++++ test_regress/t/t_slice_cond_2d_side_effect.v | 36 ++++++++++++ test_regress/t/t_slice_cond_side_effect.py | 18 ++++++ test_regress/t/t_slice_cond_side_effect.v | 35 ++++++++++++ 6 files changed, 150 insertions(+), 18 deletions(-) create mode 100755 test_regress/t/t_slice_cond_2d_side_effect.py create mode 100644 test_regress/t/t_slice_cond_2d_side_effect.v create mode 100755 test_regress/t/t_slice_cond_side_effect.py create mode 100644 test_regress/t/t_slice_cond_side_effect.v diff --git a/src/V3Slice.cpp b/src/V3Slice.cpp index ab5fdea73..5aaf4767e 100644 --- a/src/V3Slice.cpp +++ b/src/V3Slice.cpp @@ -65,7 +65,8 @@ class SliceVisitor final : public VNVisitor { bool m_okInitArray = false; // Allow InitArray children // METHODS - AstNodeExpr* cloneAndSel(AstNode* nodep, int elements, int elemIdx) { + AstNodeExpr* cloneAndSel(AstNodeExpr* const nodep, int elements, int elemIdx, + const bool needPure) { // Insert an ArraySel, except for a few special cases const AstUnpackArrayDType* const arrayp = VN_CAST(nodep->dtypep()->skipRefp(), UnpackArrayDType); @@ -79,7 +80,7 @@ class SliceVisitor final : public VNVisitor { } m_assignError = true; // Likely will cause downstream errors - return VN_AS(nodep, NodeExpr)->cloneTree(false); + return nodep->cloneTree(false, needPure); } if (arrayp->rangep()->elementsConst() != elements) { if (!m_assignError) { @@ -191,26 +192,25 @@ class SliceVisitor final : public VNVisitor { if (!newp) newp = new AstConst{nodep->fileline(), 0}; } else if (AstNodeCond* const snodep = VN_CAST(nodep, NodeCond)) { UINFO(9, " cloneCond(" << elements << "," << elemIdx << ") " << nodep << endl); - return snodep->cloneType(snodep->condp()->cloneTreePure(false), - cloneAndSel(snodep->thenp(), elements, elemIdx), - cloneAndSel(snodep->elsep(), elements, elemIdx)); + return snodep->cloneType(snodep->condp()->cloneTree(false, needPure), + cloneAndSel(snodep->thenp(), elements, elemIdx, needPure), + cloneAndSel(snodep->elsep(), elements, elemIdx, needPure)); } else if (const AstSliceSel* const snodep = VN_CAST(nodep, SliceSel)) { UINFO(9, " cloneSliceSel(" << elements << "," << elemIdx << ") " << nodep << endl); const int leOffset = (snodep->declRange().lo() + (!snodep->declRange().ascending() ? snodep->declRange().elements() - 1 - elemIdx : elemIdx)); - newp = new AstArraySel{nodep->fileline(), snodep->fromp()->cloneTreePure(false), + newp = new AstArraySel{nodep->fileline(), snodep->fromp()->cloneTree(false, needPure), leOffset}; - } else if (VN_IS(nodep, ArraySel) || VN_IS(nodep, NodeVarRef) || VN_IS(nodep, NodeSel) - || VN_IS(nodep, CMethodHard) || VN_IS(nodep, MemberSel) - || VN_IS(nodep, ExprStmt) || VN_IS(nodep, StructSel)) { + } else if (VN_IS(nodep, NodeVarRef) || VN_IS(nodep, NodeSel) || VN_IS(nodep, CMethodHard) + || VN_IS(nodep, MemberSel) || VN_IS(nodep, ExprStmt) + || VN_IS(nodep, StructSel)) { UINFO(9, " cloneSel(" << elements << "," << elemIdx << ") " << nodep << endl); const int leOffset = !arrayp->rangep()->ascending() ? arrayp->rangep()->elementsConst() - 1 - elemIdx : elemIdx; - newp = new AstArraySel{nodep->fileline(), VN_AS(nodep, NodeExpr)->cloneTreePure(false), - leOffset}; + newp = new AstArraySel{nodep->fileline(), nodep->cloneTree(false, needPure), leOffset}; } else { if (!m_assignError) { nodep->v3error(nodep->prettyTypeName() @@ -218,7 +218,7 @@ class SliceVisitor final : public VNVisitor { } m_assignError = true; // Likely will cause downstream errors - newp = VN_AS(nodep, NodeExpr)->cloneTree(false); + newp = nodep->cloneTree(false, needPure); } return newp; } @@ -249,9 +249,20 @@ class SliceVisitor final : public VNVisitor { AstNodeAssign* newlistp = nullptr; const int elements = arrayp->rangep()->elementsConst(); for (int elemIdx = 0; elemIdx < elements; ++elemIdx) { + // Original node is replaced, so it is safe to copy it one time even if it is impure. AstNodeAssign* const newp - = nodep->cloneType(cloneAndSel(nodep->lhsp(), elements, elemIdx), - cloneAndSel(nodep->rhsp(), elements, elemIdx)); + = nodep->cloneType(cloneAndSel(nodep->lhsp(), elements, elemIdx, elemIdx != 0), + cloneAndSel(nodep->rhsp(), elements, elemIdx, elemIdx != 0)); + if (elemIdx == 0) { + nodep->foreach([this](AstExprStmt* const exprp) { + // Result expression is always evaluated to the same value, so the statements + // can be removed once they were included in the expression created for the 1st + // element. + AstNodeExpr* const resultp = exprp->resultp()->unlinkFrBack(); + exprp->replaceWith(resultp); + VL_DO_DANGLING(pushDeletep(exprp), exprp); + }); + } if (debug() >= 9) newp->dumpTree("- new: "); newlistp = AstNode::addNext(newlistp, newp); } @@ -318,10 +329,24 @@ class SliceVisitor final : public VNVisitor { const int elements = adtypep->rangep()->elementsConst(); for (int elemIdx = 0; elemIdx < elements; ++elemIdx) { // EQ(a,b) -> LOGAND(EQ(ARRAYSEL(a,0), ARRAYSEL(b,0)), ...[1]) + // Original node is replaced, so it is safe to copy it one time even if it is + // impure. AstNodeBiop* const clonep - = VN_AS(nodep->cloneType(cloneAndSel(nodep->lhsp(), elements, elemIdx), - cloneAndSel(nodep->rhsp(), elements, elemIdx)), + = VN_AS(nodep->cloneType( + cloneAndSel(nodep->lhsp(), elements, elemIdx, elemIdx != 0), + cloneAndSel(nodep->rhsp(), elements, elemIdx, elemIdx != 0)), NodeBiop); + if (elemIdx == 0) { + nodep->foreach([this](AstExprStmt* const exprp) { + // Result expression is always evaluated to the same value, so the + // statements can be removed once they were included in the expression + // created for the 1st element. + AstNodeExpr* const resultp = exprp->resultp()->unlinkFrBack(); + exprp->replaceWith(resultp); + VL_DO_DANGLING(pushDeletep(exprp), exprp); + }); + } + if (!logp) { logp = clonep; } else { diff --git a/src/astgen b/src/astgen index 215364a28..d5cb5edcb 100755 --- a/src/astgen +++ b/src/astgen @@ -1033,8 +1033,8 @@ def write_ast_macros(filename): Ast{t}* unlinkFrBackWithNext(VNRelinker* linkerp = nullptr) {{ return static_cast(AstNode::unlinkFrBackWithNext(linkerp)); }} - Ast{t}* cloneTree(bool cloneNext) {{ - return static_cast(AstNode::cloneTree(cloneNext)); + Ast{t}* cloneTree(bool cloneNext, bool needPure = false) {{ + return static_cast(AstNode::cloneTree(cloneNext, needPure)); }} Ast{t}* cloneTreePure(bool cloneNext) {{ return static_cast(AstNode::cloneTreePure(cloneNext)); diff --git a/test_regress/t/t_slice_cond_2d_side_effect.py b/test_regress/t/t_slice_cond_2d_side_effect.py new file mode 100755 index 000000000..d4f986441 --- /dev/null +++ b/test_regress/t/t_slice_cond_2d_side_effect.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') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_slice_cond_2d_side_effect.v b/test_regress/t/t_slice_cond_2d_side_effect.v new file mode 100644 index 000000000..06f150340 --- /dev/null +++ b/test_regress/t/t_slice_cond_2d_side_effect.v @@ -0,0 +1,36 @@ +// 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 + +typedef int arr_t[5][3]; + +class Cls; + int cnt; + int init_depth; + function arr_t get_arr(int depth); + arr_t arr = (depth > 1) ? get_arr(depth - 1) : '{5{'{init_depth, init_depth * 2, init_depth * 3}}}; + cnt++; + return arr; + endfunction +endclass + +module t (/*AUTOARG*/); + Cls c = new; + initial begin + arr_t arr; + c.init_depth = 5; + arr = (c.init_depth > 0) ? c.get_arr(5) : '{5{'{1, 2, 3}}}; + + if (arr[0][0] != 5) $stop; + if (arr[0][1] != 10) $stop; + if (arr[0][2] != 15) $stop; + if (arr[3][2] != 15) $stop; + + if (c.cnt != 5) $stop; + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_slice_cond_side_effect.py b/test_regress/t/t_slice_cond_side_effect.py new file mode 100755 index 000000000..d4f986441 --- /dev/null +++ b/test_regress/t/t_slice_cond_side_effect.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') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_slice_cond_side_effect.v b/test_regress/t/t_slice_cond_side_effect.v new file mode 100644 index 000000000..4c2bc092e --- /dev/null +++ b/test_regress/t/t_slice_cond_side_effect.v @@ -0,0 +1,35 @@ +// 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 + +typedef int arr_t[3]; + +class Cls; + int cnt; + int init_depth; + function arr_t get_arr(int depth); + arr_t arr = (depth > 1) ? get_arr(depth - 1) : '{init_depth, init_depth * 2, init_depth * 3}; + cnt++; + return arr; + endfunction +endclass + +module t (/*AUTOARG*/); + Cls c = new; + initial begin + arr_t arr; + c.init_depth = 5; + arr = (c.init_depth > 0) ? c.get_arr(5) : '{1, 2, 3}; + + if (arr[0] != 5) $stop; + if (arr[1] != 10) $stop; + if (arr[2] != 15) $stop; + + if (c.cnt != 5) $stop; + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule