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

@@ -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<std::string, AstVar*> 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<WriteReference> 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"
+        const std::string baseName = uniqueTmpName(scopep, vscp, vscpInfo);
-                                   + std::to_string(vscpInfo.m_nTmp++)};
 
         // 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"
+        const std::string baseName = uniqueTmpName(scopep, vscp, vscpInfo);
-                                   + std::to_string(vscpInfo.m_nTmp++)};
 
         // 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"
+        const std::string baseName = uniqueTmpName(scopep, vscp, vscpInfo);
-                                   + std::to_string(vscpInfo.m_nTmp++)};
 
         // 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");

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

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