From 2958a5aaaea15db1a7c11e280397487a5fc5aff8 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Wed, 23 Jul 2025 10:48:55 +0100 Subject: [PATCH] Internals: Do not emit temporaries for atomic assignments. (#6217) Added test for a particularly convoluted case requiring fixup in V3Premit. To help with statistics stability, also prevent V3Premit from introducing temporaries for assignment where the RHS reads the LHS, but the assignment is known to be atomic (by emitted C++ semantics). Also rename `createWideTemp` to `createTemp`, as it is used for non-wide expressions as well. --- src/V3Premit.cpp | 23 ++++++++++++-------- test_regress/t/t_wide_temp_while_cond.cpp | 12 +++++++++++ test_regress/t/t_wide_temp_while_cond.py | 22 +++++++++++++++++++ test_regress/t/t_wide_temp_while_cond.v | 26 +++++++++++++++++++++++ 4 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 test_regress/t/t_wide_temp_while_cond.cpp create mode 100755 test_regress/t/t_wide_temp_while_cond.py create mode 100644 test_regress/t/t_wide_temp_while_cond.v diff --git a/src/V3Premit.cpp b/src/V3Premit.cpp index a9d7f9581..d9b5f805f 100644 --- a/src/V3Premit.cpp +++ b/src/V3Premit.cpp @@ -46,6 +46,7 @@ class PremitVisitor final : public VNVisitor { // STATE - across all visitors VDouble0 m_extractedToConstPool; // Statistic tracking + VDouble0 m_temporaryVarsCreated; // Statistic tracking // STATE - for current visit position (use VL_RESTORER) AstCFunc* m_cfuncp = nullptr; // Current block @@ -62,12 +63,12 @@ class PremitVisitor final : public VNVisitor { if (!nodep->isWide()) return; // Not wide if (m_assignLhs) return; // This is an lvalue! UASSERT_OBJ(!VN_IS(nodep->firstAbovep(), ArraySel), nodep, "Should have been ignored"); - createWideTemp(nodep); + createTemp(nodep); } - AstVar* createWideTemp(AstNodeExpr* nodep) { + AstVar* createTemp(AstNodeExpr* nodep) { UASSERT_OBJ(m_stmtp, nodep, "Attempting to create temporary with no insertion point"); - UINFO(4, "createWideTemp: " << nodep); + UINFO(4, "createTemp: " << nodep); VNRelinker relinker; nodep->unlinkFrBack(&relinker); @@ -93,6 +94,7 @@ class PremitVisitor final : public VNVisitor { const std::string name = "__Vtemp_" + std::to_string(++m_tmpVarCnt); varp = new AstVar{flp, VVarType::STMTTEMP, name, nodep->dtypep()}; m_cfuncp->addInitsp(varp); + ++m_temporaryVarsCreated; // Put assignment before the referencing statement assignp = new AstAssign{flp, new AstVarRef{flp, varp, VAccess::WRITE}, nodep}; @@ -221,9 +223,10 @@ class PremitVisitor final : public VNVisitor { } } - if (rhsReadsLhs(nodep)) { - // Need to do this even if not wide, as e.g. a select may be on a wide operator - createWideTemp(nodep->rhsp()); + // If the RHS reads the LHS, we need a temporary unless the update is atomic + const bool isAtomic = VN_IS(nodep->lhsp(), VarRef) && !nodep->lhsp()->isWide(); + if (!isAtomic && rhsReadsLhs(nodep)) { + createTemp(nodep->rhsp()); } else { iterateAndNextNull(nodep->rhsp()); } @@ -290,7 +293,7 @@ class PremitVisitor final : public VNVisitor { void visit(AstCvtPackedToArray* nodep) override { iterateChildren(nodep); checkNode(nodep); - if (!VN_IS(nodep->backp(), NodeAssign)) createWideTemp(nodep); + if (!VN_IS(nodep->backp(), NodeAssign)) createTemp(nodep); } void visit(AstCvtUnpackedToQueue* nodep) override { iterateChildren(nodep); @@ -330,7 +333,7 @@ class PremitVisitor final : public VNVisitor { && !VN_IS(nodep->condp(), VarRef)) { // We're going to need the expression several times in the expanded code, // so might as well make it a common expression - createWideTemp(nodep->condp()); + createTemp(nodep->condp()); VIsCached::clearCacheTree(); } checkNode(nodep); @@ -342,7 +345,7 @@ class PremitVisitor final : public VNVisitor { for (AstNodeExpr *expp = nodep->exprsp(), *nextp; expp; expp = nextp) { nextp = VN_AS(expp->nextp(), NodeExpr); if (expp->isString() && !VN_IS(expp, VarRef)) { - AstVar* const varp = createWideTemp(expp); + AstVar* const varp = createTemp(expp); // Do not remove VarRefs to this in V3Const varp->noSubst(true); } @@ -360,6 +363,8 @@ public: ~PremitVisitor() override { V3Stats::addStat("Optimizations, Prelim extracted value to ConstPool", m_extractedToConstPool); + V3Stats::addStat("Optimizations, Prelim temporary variables created", + m_temporaryVarsCreated); } }; diff --git a/test_regress/t/t_wide_temp_while_cond.cpp b/test_regress/t/t_wide_temp_while_cond.cpp new file mode 100644 index 000000000..d6e745f07 --- /dev/null +++ b/test_regress/t/t_wide_temp_while_cond.cpp @@ -0,0 +1,12 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +// +// DESCRIPTION: Verilator: Verilog Test module +// +// Copyright 2025 by Geza Lore. 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 +//************************************************************************* + +extern "C" int identity(int x) { return x; } diff --git a/test_regress/t/t_wide_temp_while_cond.py b/test_regress/t/t_wide_temp_while_cond.py new file mode 100755 index 000000000..9df9caa86 --- /dev/null +++ b/test_regress/t/t_wide_temp_while_cond.py @@ -0,0 +1,22 @@ +#!/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( + verilator_flags2=["--exe", "--main", "--timing", "--stats", "t/t_wide_temp_while_cond.cpp"]) + +if test.vlt or test.vltmt: + test.file_grep(test.stats, r'Optimizations, Prelim temporary variables created\s+(\d+)', 3) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_wide_temp_while_cond.v b/test_regress/t/t_wide_temp_while_cond.v new file mode 100644 index 000000000..2a88840c4 --- /dev/null +++ b/test_regress/t/t_wide_temp_while_cond.v @@ -0,0 +1,26 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Geza Lore. +// SPDX-License-Identifier: CC0-1.0 + +import "DPI-C" pure function int identity(input int value); + +module t; + initial begin + int n = 0; + logic [127:0] val = 128'b1; + logic [15:0] one = 16'b1; + + // This condition involves multiple wide temporaries, and an over-width + // shift, all of which requires V3Premit to fix up. + while (|((val[ 7'(one >> identity(32)) +: 96] << n) >> n)) begin + ++n; + end + + $display("n=%0d", n); + if (n != 96) $stop; + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule