From f6edf26eb2c2cd46fe74fb841c0e5ac004876681 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sun, 17 Aug 2025 19:35:40 +0100 Subject: [PATCH] Fix hierarchical NBAs (#6286) (#6300) NBAs targeting a variable in a different scope are now allocated temporary variables for captured values in the scope of the NBA, not the scope of the target variable. Fixes #6286 --- src/V3Delayed.cpp | 46 +++++++++++---- test_regress/t/t_nba_hier.py | 24 ++++++++ test_regress/t/t_nba_hier.v | 107 +++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 10 deletions(-) create mode 100755 test_regress/t/t_nba_hier.py create mode 100644 test_regress/t/t_nba_hier.v diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index 0ba1758d9..1e7a07c53 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -245,7 +245,9 @@ class DelayedVisitor final : public VNVisitor { // AstVar::user1() -> bool. Set true if already issued MULTIDRIVEN warning // AstVarRef::user1() -> bool. Set true if target of NBA // AstAssignDly::user1() -> bool. Set true if already visited + // AstAssignDly::user2p() -> AstVarScope*: Scope this AstAssignDelay is under // AstNodeModule::user1p() -> std::unorded_map temp map via m_varMap + // AstScope::user1() -> int: Temporary counter for this scope // AstVarScope::user1p() -> VarScopeInfo via m_vscpInfo // AstVarScope::user2p() -> AstVarRef*: First write reference to the Variable // AstVarScope::user3p() -> std::vector via m_writeRefs; @@ -263,6 +265,7 @@ class DelayedVisitor final : public VNVisitor { AstActive* m_activep = nullptr; // Current activate const AstCFunc* m_cfuncp = nullptr; // Current public C Function AstNodeProcedure* m_procp = nullptr; // Current process + AstScope* m_scopep = nullptr; // Current scope bool m_inLoop = false; // True in for loops bool m_inSuspendableOrFork = false; // True in suspendable processes and forks bool m_ignoreBlkAndNBlk = false; // Suppress delayed assignment BLKANDNBLK @@ -466,6 +469,7 @@ class DelayedVisitor final : public VNVisitor { varp = new AstVar{flp, VVarType::BLOCKTEMP, name, dtypep}; modp->addStmtsp(varp); } + UASSERT_OBJ(varp->dtypep()->isSame(dtypep), flp, "Invalid type for temporary AstVar"); // Create the AstVarScope AstVarScope* const varscp = new AstVarScope{flp, scopep, varp}; scopep->addVarsp(varscp); @@ -531,6 +535,21 @@ class DelayedVisitor final : public VNVisitor { return lhsp; } + // Create a unique temporary variable name + std::string uniqueTmpName(AstScope* scopep, AstVarScope* vscp, VarScopeInfo& vscpInfo) { + std::stringstream ss; + ss << "__" << vscp->varp()->shortName() + "__v"; + // If the assignment is in the same scope as the variable, just + // use the temporary counter of the variable. + if (scopep == vscp->scopep()) { + ss << vscpInfo.m_nTmp++; + } else { + // Otherwise use the temporary counter of the scope of the assignment. + ss << scopep->user1Inc() << "_hierarchical"; + } + return ss.str(); + } + // Create a temporary variable in the given 'scopep', with the given 'name', and with 'dtypep' // type, with the bits selected by 'sLsbp'/'sWidthp' set to 'valuep', other bits set to zero. // Insert new statements before 'insertp'. @@ -671,11 +690,10 @@ class DelayedVisitor final : public VNVisitor { void convertSchemeFlagShared(AstAssignDly* nodep, AstVarScope* vscp, VarScopeInfo& vscpInfo) { UASSERT_OBJ(vscpInfo.m_scheme == Scheme::FlagShared, vscp, "Inconsistent NBA scheme"); FileLine* const flp = vscp->fileline(); - AstScope* const scopep = vscp->scopep(); + AstScope* const scopep = VN_AS(nodep->user2p(), Scope); // Base name suffix for signals constructed below - const std::string baseName{"__" + vscp->varp()->shortName() + "__v" - + std::to_string(vscpInfo.m_nTmp++)}; + const std::string baseName = uniqueTmpName(scopep, vscp, vscpInfo); // Unlink and capture the RHS value AstNodeExpr* const capturedRhsp @@ -697,6 +715,8 @@ class DelayedVisitor final : public VNVisitor { consecutive // ... that use the same scheme && prevVscpInfop->m_scheme == Scheme::FlagShared + // ... and are in the same scope as the target variable + && scopep == vscp->scopep() // ... and share the same overall update domain && prevVscpInfop->senTreep()->sameTree(vscpInfo.senTreep()); @@ -763,11 +783,10 @@ class DelayedVisitor final : public VNVisitor { void convertSchemeFlagUnique(AstAssignDly* nodep, AstVarScope* vscp, VarScopeInfo& vscpInfo) { UASSERT_OBJ(vscpInfo.m_scheme == Scheme::FlagUnique, vscp, "Inconsistent NBA scheme"); FileLine* const flp = vscp->fileline(); - AstScope* const scopep = vscp->scopep(); + AstScope* const scopep = VN_AS(nodep->user2p(), Scope); // Base name suffix for signals constructed below - const std::string baseName{"__" + vscp->varp()->shortName() + "__v" - + std::to_string(vscpInfo.m_nTmp++)}; + const std::string baseName = uniqueTmpName(scopep, vscp, vscpInfo); // Unlink and capture the RHS value AstNodeExpr* const capturedRhsp @@ -837,11 +856,10 @@ class DelayedVisitor final : public VNVisitor { : vscpInfo.m_scheme == Scheme::ValueQueueWhole, vscp, "Inconsistent NBA scheme"); FileLine* const flp = vscp->fileline(); - AstScope* const scopep = vscp->scopep(); + AstScope* const scopep = VN_AS(nodep->user2p(), Scope); // Base name suffix for signals constructed below - const std::string baseName{"__" + vscp->varp()->shortName() + "__v" - + std::to_string(vscpInfo.m_nTmp++)}; + const std::string baseName = uniqueTmpName(scopep, vscp, vscpInfo); // Unlink and capture the RHS value AstNodeExpr* const capturedRhsp @@ -1051,7 +1069,11 @@ class DelayedVisitor final : public VNVisitor { } } } - void visit(AstScope* nodep) override { iterateChildren(nodep); } + void visit(AstScope* nodep) override { + VL_RESTORER(m_scopep); + m_scopep = nodep; + iterateChildren(nodep); + } void visit(AstCFunc* nodep) override { VL_RESTORER(m_cfuncp); m_cfuncp = nodep; @@ -1169,9 +1191,13 @@ class DelayedVisitor final : public VNVisitor { } UASSERT_OBJ(m_procp, nodep, "Delayed assignment not under process"); UASSERT_OBJ(m_activep, nodep, "<= not under sensitivity block"); + UASSERT_OBJ(m_scopep, nodep, "<= not under scope"); UASSERT_OBJ(m_inSuspendableOrFork || m_activep->hasClocked(), nodep, "<= assignment in non-clocked block, should have been converted in V3Active"); + // Record scope of this NBA + nodep->user2p(m_scopep); + // Grab the reference to the target of the NBA, also lift ExprStmt statements on the LHS VL_RESTORER(m_currNbaLhsRefp); UASSERT_OBJ(!m_currNbaLhsRefp, nodep, "NBAs should not nest"); diff --git a/test_regress/t/t_nba_hier.py b/test_regress/t/t_nba_hier.py new file mode 100755 index 000000000..5714e793e --- /dev/null +++ b/test_regress/t/t_nba_hier.py @@ -0,0 +1,24 @@ +#!/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=['--binary', '--stats', '-fno-inline', '--unroll-count', '0']) + +test.file_grep(test.stats, r'NBA, variables using ShadowVar scheme\s+(\d+)', 2) +test.file_grep(test.stats, r'NBA, variables using ShadowVarMasked scheme\s+(\d+)', 2) +test.file_grep(test.stats, r'NBA, variables using FlagShared scheme\s+(\d+)', 2) +test.file_grep(test.stats, r'NBA, variables using FlagUnique scheme\s+(\d+)', 2) +test.file_grep(test.stats, r'NBA, variables using ValueQueuePartial scheme\s+(\d+)', 2) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_nba_hier.v b/test_regress/t/t_nba_hier.v new file mode 100644 index 000000000..40db25713 --- /dev/null +++ b/test_regress/t/t_nba_hier.v @@ -0,0 +1,107 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2020 by Geza Lore. +// SPDX-License-Identifier: CC0-1.0 + +`define stop $stop +`define checkh(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0x exp=%0x (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); + +module t; + + logic clk = 1'b0; + always #5 clk = ~clk; + + logic [7:0] x; + + sub a_0(); + sub a_1(); + always @(posedge clk) begin + a_0.x[3:0] <= ~x[3:0]; + a_1.x[7:0] <= ~x; + end + + sub b_0(); + sub b_1(); + always begin + // Having this @posedge here makes this a 'suspendable' process, causing + // the use of the FlagUnique scheme + @(posedge clk); + b_0.x[3:0] <= ~x[3:0]; + b_1.x[7:0] <= ~x; + end + + sub c_0(); + sub c_1(); + always @(posedge clk) begin + c_0.x[3:0] <= ~x[3:0]; + c_1.x[7:0] <= ~x; + end + assign c_0.x[9:8] = 2'd1; + assign c_1.x[9:8] = 2'd2; + + sub d_0(); + sub d_1(); + always @(posedge clk) begin + d_0.y[0][3:0] <= ~x[3:0]; + d_1.y[0][7:0] <= ~x; + end + + sub e_0(); + sub e_1(); + always @(posedge clk) begin + for (int i = 0; i < 2; ++i) begin + e_0.y[i][3:0] <= ~x[3:0]; + e_1.y[i][7:0] <= ~x; + end + end + + initial begin + #1; + x = 8'hcc; + @(posedge clk); + @(negedge clk); + `checkh(a_0.x[3:0], 4'h3); + `checkh(a_1.x[7:0], 8'h33); + `checkh(b_0.x[3:0], 4'h3); + `checkh(b_1.x[7:0], 8'h33); + `checkh(c_0.x[3:0], 4'h3); + `checkh(c_0.x[9:8], 2'h1); + `checkh(c_1.x[7:0], 8'h33); + `checkh(c_1.x[9:8], 2'h2); + `checkh(d_0.y[0][3:0], 4'h3); + `checkh(d_1.y[0][7:0], 8'h33); + for (int i = 0; i < 2; ++i) begin + `checkh(e_0.y[i][3:0], 4'h3); + `checkh(e_1.y[i][7:0], 8'h33); + end + + #1; + x = 8'h55; + @(posedge clk); + @(negedge clk); + `checkh(a_0.x[3:0], 4'ha); + `checkh(a_1.x[7:0], 8'haa); + `checkh(b_0.x[3:0], 4'ha); + `checkh(b_1.x[7:0], 8'haa); + `checkh(c_0.x[3:0], 4'ha); + `checkh(c_0.x[9:8], 2'h1); + `checkh(c_1.x[7:0], 8'haa); + `checkh(c_1.x[9:8], 2'h2); + `checkh(d_0.y[0][3:0], 4'ha); + `checkh(d_1.y[0][7:0], 8'haa); + for (int i = 0; i < 2; ++i) begin + `checkh(e_0.y[i][3:0], 4'ha); + `checkh(e_1.y[i][7:0], 8'haa); + end + + #1; + $finish; + end + +endmodule + +module sub; + logic [9:0] x; + logic [9:0] y [99]; +endmodule