Fix false ASSIGNIN on functions with explicit port map (#5069).

Changes

@@ -18,6 +18,7 @@ Verilator 5.025 devel
 * Fix consecutive zero-delays (#5038). [Krzysztof Bieganski, Antmicro Ltd.]
 * Fix `$system` with string argument (#5042).
 * Fix width extension on delays (#5043).
+* Fix false ASSIGNIN on functions with explicit port map (#5069).
 
 
 Verilator 5.024 2024-04-05

src/V3LinkLValue.cpp

@@ -22,6 +22,8 @@
 
 #include "V3LinkLValue.h"
 
+#include "V3Task.h"
+
 VL_DEFINE_DEBUG_FUNCTIONS;
 
 //######################################################################
@@ -279,13 +281,17 @@ class LinkLValueVisitor final : public VNVisitor {
         iterateChildren(nodep);
     }
     void visit(AstNodeFTaskRef* nodep) override {
-        AstNode* pinp = nodep->pinsp();
         const AstNodeFTask* const taskp = nodep->taskp();
         // We'll deal with mismatching pins later
         if (!taskp) return;
-        for (AstNode* stmtp = taskp->stmtsp(); stmtp && pinp; stmtp = stmtp->nextp()) {
+        const V3TaskConnects tconnects
-            if (const AstVar* const portp = VN_CAST(stmtp, Var)) {
+            = V3Task::taskConnects(nodep, taskp->stmtsp(), nullptr, false);
-                if (portp->isIO()) {
+        for (const auto& tconnect : tconnects) {
+            const AstVar* const portp = tconnect.first;
+            const AstArg* const argp = tconnect.second;
+            if (!argp) continue;
+            AstNodeExpr* const pinp = argp->exprp();
+            if (!pinp) continue;
             if (portp->isWritable()) {
                 m_setRefLvalue = VAccess::WRITE;
                 iterate(pinp);
@@ -293,10 +299,6 @@ class LinkLValueVisitor final : public VNVisitor {
             } else {
                 iterate(pinp);
             }
-                    // Advance pin
-                    pinp = pinp->nextp();
-                }
-            }
         }
     }
 

src/V3Task.cpp

@@ -1589,14 +1589,13 @@ public:
 const char* const V3Task::s_dpiTemporaryVarSuffix = "__Vcvt";
 
 V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
-                                    V3TaskConnectState* statep) {
+                                    V3TaskConnectState* statep, bool makeChanges) {
     // Output list will be in order of the port declaration variables (so
     // func calls are made right in C)
     // Missing pin/expr?  We return (pinvar, nullptr)
     // Extra   pin/expr?  We clean it up
     UINFO(9, "taskConnects " << nodep << endl);
     std::map<const std::string, int> nameToIndex;
-    std::set<const AstVar*> argWrap;  // Which ports are defaulted, forcing arg wrapper creation
     V3TaskConnects tconnects;
     UASSERT_OBJ(nodep->taskp(), nodep, "unlinked");
 
@@ -1608,7 +1607,7 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
             if (portp->isIO()) {
                 tconnects.emplace_back(portp, static_cast<AstArg*>(nullptr));
                 nameToIndex.emplace(portp->name(), tpinnum);  // For name based connections
-                tpinnum++;
+                ++tpinnum;
                 if (portp->attrSFormat()) {
                     sformatp = portp;
                 } else if (sformatp) {
@@ -1630,17 +1629,19 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
             // By name
             const auto it = nameToIndex.find(argp->name());
             if (it == nameToIndex.end()) {
-                pinp->v3error("No such argument " << argp->prettyNameQ() << " in function call to "
+                if (makeChanges) {
+                    pinp->v3error("No such argument " << argp->prettyNameQ()
+                                                      << " in function call to "
                                                       << nodep->taskp()->prettyTypeName());
                     // We'll just delete it; seems less error prone than making a false argument
                     VL_DO_DANGLING(pinp->unlinkFrBack()->deleteTree(), pinp);
+                }
             } else {
-                if (tconnects[it->second].second) {
+                if (tconnects[it->second].second && makeChanges) {
                     pinp->v3error("Duplicate argument " << argp->prettyNameQ()
                                                         << " in function call to "
                                                         << nodep->taskp()->prettyTypeName());
                 }
-                argp->name("");  // Can forget name as will add back in pin order
                 tconnects[it->second].second = argp;
                 reorganize = true;
             }
@@ -1649,8 +1650,8 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
                 if (sformatp) {
                     tconnects.emplace_back(sformatp, static_cast<AstArg*>(nullptr));
                     tconnects[ppinnum].second = argp;
-                    tpinnum++;
+                    ++tpinnum;
-                } else {
+                } else if (makeChanges) {
                     pinp->v3error("Too many arguments in function call to "
                                   << nodep->taskp()->prettyTypeName());
                     // We'll just delete it; seems less error prone than making a false argument
@@ -1660,10 +1661,13 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
                 tconnects[ppinnum].second = argp;
             }
         }
-        ppinnum++;
+        ++ppinnum;
     }
 
+    if (!makeChanges) return tconnects;
+
     // Connect missing ones
+    std::set<const AstVar*> argWrap;  // Which ports are defaulted, forcing arg wrapper creation
     for (int i = 0; i < tpinnum; ++i) {
         AstVar* const portp = tconnects[i].first;
         if (!tconnects[i].second || !tconnects[i].second->exprp()) {
@@ -1723,6 +1727,11 @@ V3TaskConnects V3Task::taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
         }
     }
 
+    for (const auto& tconnect : tconnects) {
+        AstArg* const argp = tconnect.second;
+        argp->name("");  // Can forget name as will add back in pin order
+    }
+
     if (reorganize) {
         // To simplify downstream, put argument list back into pure pinnumber ordering
         while (nodep->pinsp()) {

src/V3Task.h

@@ -55,7 +55,8 @@ public:
     static void taskAll(AstNetlist* nodep) VL_MT_DISABLED;
     /// Return vector of [port, pin-connects-to]  (SLOW)
     static V3TaskConnects taskConnects(AstNodeFTaskRef* nodep, AstNode* taskStmtsp,
-                                       V3TaskConnectState* statep = nullptr) VL_MT_DISABLED;
+                                       V3TaskConnectState* statep = nullptr,
+                                       bool makeChanges = true) VL_MT_DISABLED;
     static void taskConnectWrap(AstNodeFTaskRef* nodep, const V3TaskConnects& tconnects,
                                 V3TaskConnectState* statep,
                                 const std::set<const AstVar*>& argWrap) VL_MT_DISABLED;

test_regress/t/t_func_io_order.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(simulator => 1);
+
+compile(
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;

test_regress/t/t_func_io_order.v

@@ -0,0 +1,75 @@
+// 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
+
+module t(/*AUTOARG*/
+   // Inputs
+   clk
+   );
+   input clk;
+
+   integer cyc = 0;
+   reg [63:0] crc;
+   reg [63:0] sum;
+
+   // Take CRC data and apply to testblock inputs
+   wire ain = crc[0];
+
+   /*AUTOWIRE*/
+   // Beginning of automatic wires (for undeclared instantiated-module outputs)
+   logic                bout;                   // From test of Test.v
+   // End of automatics
+
+   Test test(/*AUTOINST*/
+             // Outputs
+             .bout                      (bout),
+             // Inputs
+             .ain                       (ain));
+
+   // Test loop
+   always @ (posedge clk) begin
+`ifdef TEST_VERBOSE
+      $write("[%0t] cyc==%0d crc=%x result=%x\n", $time, cyc, crc, result);
+`endif
+      cyc <= cyc + 1;
+      crc <= {crc[62:0], crc[63] ^ crc[2] ^ crc[0]};
+      if (cyc == 0) begin
+         // Setup
+         crc <= 64'h5aef0c8d_d70a4497;
+      end
+      else if (cyc < 10) begin
+         if (bout != ~ain) $stop;
+      end
+      else if (cyc == 99) begin
+         $write("[%0t] cyc==%0d crc=%x sum=%x\n", $time, cyc, crc, sum);
+         if (crc !== 64'hc77bb9b3784ea091) $stop;
+         $write("*-* All Finished *-*\n");
+         $finish;
+      end
+   end
+
+endmodule
+
+module Test(/*AUTOARG*/
+   // Outputs
+   bout,
+   // Inputs
+   ain
+   );
+
+   input logic ain;
+   output logic bout;
+
+   function automatic void inv
+     (input logic w_in,
+      output logic w_out);
+      w_out = ~w_in;
+   endfunction
+
+   always_comb
+     inv(.w_out(bout),
+         .w_in(ain));
+
+endmodule