Do not create aliases for forced port signals

+ don't remove forced signals in V3Const and Dfg Fixes verilator#5062
gezalore · May 8, 2024 · 9f77e9e · 9f77e9e
1 parent 69a2bfe
commit 9f77e9e
Show file tree

Hide file tree

Showing 14 changed files with 136 additions and 31 deletions.
diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h
@@ -1809,6 +1809,7 @@ class AstVar final : public AstNode {
     bool m_trace : 1;  // Trace this variable
     bool m_isLatched : 1;  // Not assigned in all control paths of combo always
     bool m_isForceable : 1;  // May be forced/released externally from user C code
+    bool m_isForcedByCode : 1;  // May be forced/released from AstAssignForce/AstRelease
     bool m_isWrittenByDpi : 1;  // This variable can be written by a DPI Export
     bool m_isWrittenBySuspendable : 1;  // This variable can be written by a suspendable process
 
@@ -1854,6 +1855,7 @@ class AstVar final : public AstNode {
         m_trace = false;
         m_isLatched = false;
         m_isForceable = false;
+        m_isForcedByCode = false;
         m_isWrittenByDpi = false;
         m_isWrittenBySuspendable = false;
         m_attrClocker = VVarAttrClocker::CLOCKER_UNKNOWN;
@@ -2009,6 +2011,8 @@ class AstVar final : public AstNode {
     void isLatched(bool flag) { m_isLatched = flag; }
     bool isForceable() const { return m_isForceable; }
     void setForceable() { m_isForceable = true; }
+    void setForcedByCode() { m_isForcedByCode = true; }
+    bool isForced() const { return m_isForceable || m_isForcedByCode; }
     bool isWrittenByDpi() const { return m_isWrittenByDpi; }
     void setWrittenByDpi() { m_isWrittenByDpi = true; }
     bool isWrittenBySuspendable() const { return m_isWrittenBySuspendable; }

diff --git a/src/V3Const.cpp b/src/V3Const.cpp
@@ -3071,8 +3071,9 @@ class ConstVisitor final : public VNVisitor {
             && varrefp  // Don't do messes with BITREFs/ARRAYREFs
             && !varrefp->varp()->hasStrengthAssignment()  // Strengths are resolved in V3Tristate
             && !varrefp->varp()->valuep()  // Not already constified
-            && !varrefp->varScopep()) {  // Not scoped (or each scope may have different initial
-                                         // value)
+            && !varrefp->varScopep()  // Not scoped (or each scope may have different initial val.)
+            && !varrefp->varp()->isForced()  // Not forced (not really a constant)
+        ) {
             // ASSIGNW (VARREF, const) -> INITIAL ( ASSIGN (VARREF, const) )
             UINFO(4, "constAssignW " << nodep << endl);
             // Make a initial assignment

diff --git a/src/V3DfgAstToDfg.cpp b/src/V3DfgAstToDfg.cpp
@@ -443,7 +443,7 @@ class AstToDfgVisitor final : public VNVisitor {
         // Mark variables with external references
         if (nodep->isIO()  // Ports
             || nodep->user2()  // Target of a hierarchical reference
-            || nodep->isForceable()  // Forceable
+            || nodep->isForced()  // Forced
         ) {
             getNet(nodep)->setHasExtRefs();
         }

diff --git a/src/V3DfgPasses.cpp b/src/V3DfgPasses.cpp
@@ -165,8 +165,8 @@ void V3DfgPasses::inlineVars(DfgGraph& dfg) {
                     const AstVar* const astVarp = driverVarp->varp();
                     // If driven from a SystemC variable
                     if (astVarp->isSc()) continue;
-                    // If the variable is forceable
-                    if (astVarp->isForceable()) continue;
+                    // If the variable is forced
+                    if (astVarp->isForced()) continue;
                 }
 
                 varp->forEachSinkEdge([=](DfgEdge& edge) { edge.relinkSource(driverp); });

diff --git a/src/V3DfgVertices.h b/src/V3DfgVertices.h
@@ -239,7 +239,7 @@ class DfgVarPacked final : public DfgVertexVar {
     ASTGEN_MEMBERS_DfgVarPacked;
 
     bool isDrivenFullyByDfg() const {
-        return arity() == 1 && source(0)->dtypep() == dtypep() && !varp()->isForceable();
+        return arity() == 1 && source(0)->dtypep() == dtypep() && !varp()->isForced();
     }
 
     void addDriver(FileLine* flp, uint32_t lsb, DfgVertex* vtxp) {

diff --git a/src/V3Force.cpp b/src/V3Force.cpp
@@ -64,15 +64,6 @@ class ForceConvertVisitor final : public VNVisitor {
             m_rdVarp->addNext(m_enVarp);
             m_rdVarp->addNext(m_valVarp);
             varp->addNextHere(m_rdVarp);
-
-            if (varp->isPrimaryIO()) {
-                varp->v3warn(
-                    E_UNSUPPORTED,
-                    "Unsupported: Force/Release on primary input/output net "
-                        << varp->prettyNameQ() << "\n"
-                        << varp->warnMore()
-                        << "... Suggest assign it to/from a temporary net and force/release that");
-            }
         }
     };
 

diff --git a/src/V3Gate.cpp b/src/V3Gate.cpp
@@ -425,8 +425,8 @@ class GateClkDecomp final {
     void visit(GateVarVertex* vVtxp, int offset) {
         AstVarScope* const vscp = vVtxp->varScp();
 
-        // Can't propagate if this variable is forceable
-        if (vscp->varp()->isForceable()) return;
+        // Can't propagate if this variable might be forced
+        if (vscp->varp()->isForced()) return;
 
         // Check that we haven't been here before
         if (vscp->user2SetOnce()) return;

diff --git a/src/V3Inline.cpp b/src/V3Inline.cpp
@@ -320,8 +320,16 @@ class InlineRelinkVisitor final : public VNVisitor {
                 // module, so a AstVarRef not AstVarXRef below
                 exprvarrefp = exprvarrefp->cloneTree(false);
                 exprvarrefp->access(VAccess::READ);
-                m_modp->addStmtsp(new AstAssignAlias{
-                    flp, new AstVarRef{flp, nodep, VAccess::WRITE}, exprvarrefp});
+                AstVarRef* const nodeVarRefp = new AstVarRef{flp, nodep, VAccess::WRITE};
+                if (nodep->isForced() && nodep->direction() == VDirection::INPUT) {
+                    m_modp->addStmtsp(new AstAssignW{flp, nodeVarRefp, exprvarrefp});
+                } else if (nodep->isForced() && nodep->direction() == VDirection::OUTPUT) {
+                    exprvarrefp->access(VAccess::WRITE);
+                    nodeVarRefp->access(VAccess::READ);
+                    m_modp->addStmtsp(new AstAssignW{flp, exprvarrefp, nodeVarRefp});
+                } else {
+                    m_modp->addStmtsp(new AstAssignAlias{flp, nodeVarRefp, exprvarrefp});
+                }
                 FileLine* const flbp = exprvarrefp->varp()->fileline();
                 flp->modifyStateInherit(flbp);
                 flbp->modifyStateInherit(flp);
@@ -370,7 +378,9 @@ class InlineRelinkVisitor final : public VNVisitor {
         if (nodep->varp()->user2p()  // It's being converted to an alias.
             && !nodep->varp()->user3()
             // Don't constant propagate aliases (we just made)
-            && !VN_IS(nodep->backp(), AssignAlias)) {
+            && !VN_IS(nodep->backp(), AssignAlias)
+            // Forced signals do not use aliases
+            && !nodep->varp()->isForced()) {
             AstVar* const varp = nodep->varp();
             if (AstConst* const constp = VN_CAST(varp->user2p(), Const)) {
                 nodep->replaceWith(constp->cloneTree(false));

diff --git a/src/V3LinkLValue.cpp b/src/V3LinkLValue.cpp
@@ -35,25 +35,26 @@ class LinkLValueVisitor final : public VNVisitor {
     // STATE
     bool m_setContinuously = false;  // Set that var has some continuous assignment
     bool m_setStrengthSpecified = false;  // Set that var has assignment with strength specified.
+    bool m_setForcedByCode = false;  // Set that var is the target of an AstAssignForce/AstRelease
     VAccess m_setRefLvalue;  // Set VarRefs to lvalues for pin assignments
 
     // VISITs
     // Result handing
     void visit(AstNodeVarRef* nodep) override {
         // VarRef: LValue its reference
         if (m_setRefLvalue != VAccess::NOCHANGE) nodep->access(m_setRefLvalue);
-        if (nodep->varp()) {
-            if (nodep->access().isWriteOrRW() && m_setContinuously) {
+        if (nodep->varp() && nodep->access().isWriteOrRW()) {
+            if (m_setContinuously) {
                 nodep->varp()->isContinuously(true);
                 // Strength may only be specified in continuous assignment,
                 // so it is needed to check only if m_setContinuously is true
                 if (m_setStrengthSpecified) nodep->varp()->hasStrengthAssignment(true);
             }
-            if (nodep->access().isWriteOrRW() && !nodep->varp()->isFuncLocal()
-                && nodep->varp()->isReadOnly()) {
+            if (!nodep->varp()->isFuncLocal() && nodep->varp()->isReadOnly()) {
                 nodep->v3warn(ASSIGNIN,
                               "Assigning to input/const variable: " << nodep->prettyNameQ());
             }
+            if (m_setForcedByCode) nodep->varp()->setForcedByCode();
         }
         iterateChildren(nodep);
     }
@@ -79,7 +80,11 @@ class LinkLValueVisitor final : public VNVisitor {
             if (AstAssignW* assignwp = VN_CAST(nodep, AssignW)) {
                 if (assignwp->strengthSpecp()) m_setStrengthSpecified = true;
             }
-            iterateAndNextNull(nodep->lhsp());
+            {
+                VL_RESTORER(m_setForcedByCode);
+                m_setForcedByCode = VN_IS(nodep, AssignForce);
+                iterateAndNextNull(nodep->lhsp());
+            }
             m_setRefLvalue = VAccess::NOCHANGE;
             m_setContinuously = false;
             m_setStrengthSpecified = false;
@@ -89,9 +94,11 @@ class LinkLValueVisitor final : public VNVisitor {
     void visit(AstRelease* nodep) override {
         VL_RESTORER(m_setRefLvalue);
         VL_RESTORER(m_setContinuously);
+        VL_RESTORER(m_setForcedByCode);
         {
             m_setRefLvalue = VAccess::WRITE;
             m_setContinuously = false;
+            m_setForcedByCode = true;
             iterateAndNextNull(nodep->lhsp());
         }
     }

diff --git a/test_regress/t/t_force_mid.pl b/test_regress/t/t_force_mid.pl
@@ -10,9 +10,11 @@
 
 scenarios(simulator => 1);
 
-lint(
-    fails => $Self->{vlt_all},
-    expect_filename => $Self->{golden_filename},
+compile(
+    );
+
+execute(
+      check_finised => 1,
     );
 
 ok(1);

diff --git a/test_regress/t/t_force_mid.v b/test_regress/t/t_force_mid.v
@@ -31,6 +31,10 @@ module t(/*AUTOARG*/
       end
       else if (cyc == 2) begin
          if (tried != 4'b1010) $stop;
+         release tried;
+      end
+      else if (cyc == 3) begin
+         if (tried != 4'b0101) $stop;
       end
       //
       else if (cyc == 99) begin

diff --git a/test_regress/t/t_force_port_alias.pl b/test_regress/t/t_force_port_alias.pl
@@ -0,0 +1,21 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# 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
+
+scenarios(vlt_all => 1);
+
+compile(
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;
diff --git a/test_regress/t/t_force_port_alias.v b/test_regress/t/t_force_port_alias.v
@@ -0,0 +1,61 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2024 by Wilson Snyder.
+// SPDX-License-Identifier: CC0-1.0
+
+`define stop $stop
+`define checkh(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d:  got='h%x exp='h%x\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0)
+
+module sub(
+   // Outputs
+   out,
+   // Inputs
+   clk
+   );
+   // verilator inline_module
+
+   output [3:0] out /* <-- this variable has to be marked as having external refs */;
+   input clk;
+
+   reg [3:0] r;
+
+   always @ (posedge clk)
+      r <= 4'h1;
+   assign out = r;
+endmodule
+
+module t(/*AUTOARG*/
+   // Inputs
+   clk
+   );
+
+   input clk;
+   reg [3:0] unused;
+
+   sub sub1(unused, clk);
+
+   integer cyc = 0;
+
+   always @ (posedge clk) begin
+      cyc <= cyc + 1;
+      if (cyc == 1) begin
+         `checkh(sub1.r, 4'h1);
+         `checkh(sub1.out, 4'h1);
+      end
+      else if (cyc == 2) begin
+         force sub1.r = 4'h2;
+         force sub1.out = 4'h3;
+      end
+      else if (cyc == 3) begin
+         `checkh(sub1.r, 4'h2);
+         `checkh(sub1.out, 4'h3);
+      end
+      //
+      else if (cyc == 99) begin
+         $write("*-* All Finished *-*\n");
+         $finish;
+      end
+   end
+
+endmodule
diff --git a/test_regress/t/t_force_subnet.v b/test_regress/t/t_force_subnet.v
@@ -23,15 +23,19 @@ module t(/*AUTOARG*/
       cyc <= cyc + 1;
       if (cyc == 10) begin
          `checkh(subnet, 8'h11);
-         force sub1.subnet = 8'h01;  // sub1.subnet same as subnet
+         force sub1.subnet = 8'h01;  // sub1.subnet *not* the same as subnet
       end
       else if (cyc == 11) begin
          `checkh(subnet, 8'h01);
-         force subnet = 8'h10;  // sub1.subnet same as subnet
+         force subnet = 8'h10;  // sub1.subnet *not* the same as subnet
       end
       else if (cyc == 12) begin
          `checkh(subnet, 8'h10);
-         release subnet;  // sub1.subnet same as subnet
+         release subnet;  // sub1.subnet *not* same as subnet
+      end
+      else if (cyc == 13) begin
+         `checkh(subnet, 8'h01);
+         release sub1.subnet;
       end
       else if (cyc == 13) begin
          `checkh(subnet, 8'h11);