Fix passing arguments by reference (verilator#3385 partial) (verilato…

…r#4489)

src/V3Ast.h

@@ -706,7 +706,7 @@ class VDirection final {
     bool isReadOnly() const VL_MT_SAFE { return m_e == INPUT || m_e == CONSTREF; }
     bool isWritable() const VL_MT_SAFE { return m_e == OUTPUT || m_e == INOUT || m_e == REF; }
     bool isRef() const VL_MT_SAFE { return m_e == REF; }
-    bool isRefOrConstRef() const VL_MT_SAFE { return m_e == REF || m_e == CONSTREF; }
+    bool isConstRef() const VL_MT_SAFE { return m_e == CONSTREF; }
 };
 constexpr bool operator==(const VDirection& lhs, const VDirection& rhs) {
     return lhs.m_e == rhs.m_e;

src/V3AstNodeOther.h

@@ -1931,6 +1931,7 @@ class AstVar final : public AstNode {
     bool isInoutish() const { return m_direction.isInoutish(); }
     bool isNonOutput() const { return m_direction.isNonOutput(); }
     bool isReadOnly() const VL_MT_SAFE { return m_direction.isReadOnly(); }
+    bool isConstRef() const VL_MT_SAFE { return m_direction.isConstRef(); }
     bool isRef() const VL_MT_SAFE { return m_direction.isRef(); }
     bool isWritable() const VL_MT_SAFE { return m_direction.isWritable(); }
     bool isTristate() const { return m_tristate; }

src/V3AstNodes.cpp

@@ -475,7 +475,8 @@ string AstVar::vlArgType(bool named, bool forReturn, bool forFunc, const string&
     if (isStatic() && namespc.empty()) ostatic = "static ";
 
     const bool isRef = isDpiOpenArray()
-                       || (forFunc && (isWritable() || direction().isRefOrConstRef())) || asRef;
+                       || (forFunc && (isWritable() || this->isRef() || this->isConstRef()))
+                       || asRef;
 
     if (forFunc && isReadOnly() && isRef) ostatic = ostatic + "const ";
 
@@ -589,7 +590,7 @@ string AstVar::cPubArgType(bool named, bool forReturn) const {
     if (forReturn) named = false;
     string arg;
     if (isWide() && isReadOnly()) arg += "const ";
-    const bool isRef = !forReturn && (isWritable() || direction().isRefOrConstRef());
+    const bool isRef = !forReturn && (isWritable() || this->isRef() || this->isConstRef());
     if (VN_IS(dtypeSkipRefp(), BasicDType) && !dtypeSkipRefp()->isDouble()
         && !dtypeSkipRefp()->isString()) {
         // Backward compatible type declaration

src/V3LinkLevel.cpp

@@ -277,7 +277,7 @@ void V3LinkLevel::wrapTopCell(AstNetlist* rootp) {
                     varp->sigPublic(true);  // User needs to be able to get to it...
                     oldvarp->primaryIO(false);
                     varp->primaryIO(true);
-                    if (varp->direction().isRefOrConstRef()) {
+                    if (varp->isRef() || varp->isConstRef()) {
                         varp->v3warn(E_UNSUPPORTED,
                                      "Unsupported: ref/const ref as primary input/output: "
                                          << varp->prettyNameQ());

src/V3Task.cpp

@@ -479,10 +479,38 @@ class TaskVisitor final : public VNVisitor {
                 pinp->v3error("Function/task " + portp->direction().prettyName()  // e.g. "output"
                               + " connected to constant instead of variable: "
                               + portp->prettyNameQ());
-            }
-            // else if (portp->direction() == VDirection::REF) {
-            // TODO References need to instead pass a real reference var, see issue #3385
-            else if (portp->isInoutish()) {
+            } else if (portp->isRef() || portp->isConstRef()) {
+                bool refArgOk = false;
+                if (VN_IS(pinp, VarRef) || VN_IS(pinp, MemberSel) || VN_IS(pinp, StructSel)
+                    || VN_IS(pinp, ArraySel)) {
+                    refArgOk = true;
+                } else if (const AstCMethodHard* const cMethodp = VN_CAST(pinp, CMethodHard)) {
+                    refArgOk = cMethodp->name() == "at";
+                }
+                if (refArgOk) {
+                    if (AstVarRef* const varrefp = VN_CAST(pinp, VarRef)) {
+                        varrefp->access(VAccess::READWRITE);
+                    }
+                } else {
+                    pinp->v3error("Function/task ref argument is not of allowed type");
+                }
+                if (inlineTask) {
+                    if (AstVarRef* const varrefp = VN_CAST(pinp, VarRef)) {
+                        // Connect to this exact variable
+                        AstVarScope* const localVscp = varrefp->varScopep();
+                        UASSERT_OBJ(localVscp, varrefp, "Null var scope");
+                        portp->user2p(localVscp);
+                        pushDeletep(pinp);
+                    } else {
+                        pinp->v3warn(E_TASKNSVAR, "Unsupported: ref argument of inlined "
+                                                  "function/task is not a simple variable");
+                        // Providing a var to avoid an internal error.
+                        AstVarScope* const newvscp
+                            = createVarScope(portp, namePrefix + "__" + portp->shortName());
+                        portp->user2p(newvscp);
+                    }
+                }
+            } else if (portp->isInoutish()) {
                 // if (debug() >= 9) pinp->dumpTree("-pinrsize- ");
                 V3LinkLValue::linkLValueSet(pinp);
 

test_regress/t/t_func_inout_bit_sel.pl

@@ -0,0 +1,21 @@
+#!/usr/bin/perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2003 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_inout_bit_sel.v

@@ -0,0 +1,31 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Antmicro Ltd.
+// 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);
+
+class Cls;
+   function bit get_x_set_1(inout bit x);
+      bit a = x;
+      x = 1;
+      return a;
+   endfunction
+endclass
+
+module t (/*AUTOARG*/);
+   int a;
+   bit b;
+   Cls cls;
+   initial begin
+      cls = new;
+      b = cls.get_x_set_1(a[1]);
+      `checkh(b, 0);
+      `checkh(a[1], 1);
+
+      $write("*-* All Finished *-*\n");
+      $finish;
+   end
+endmodule

test_regress/t/t_func_ref_arg.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 2020 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_ref_arg.v

@@ -0,0 +1,60 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Antmicro Ltd.
+// 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);
+
+class MyInt;
+   int x;
+   function new(int a);
+      x = a;
+   endfunction
+endclass
+
+function int get_val_set_5(ref int x);
+   automatic int y = x;
+   x = 5;
+   return y;
+endfunction
+
+class Cls;
+   function int get_val_set_2(ref int x);
+      automatic int y = x;
+      x = 2;
+      return y;
+   endfunction
+endclass
+
+module t (/*AUTOARG*/);
+   int a, b;
+   int arr[1];
+   Cls cls;
+   MyInt mi;
+   initial begin
+      a = 10;
+      b = get_val_set_5(a);
+      `checkh(a, 5);
+      `checkh(b, 10);
+
+      cls = new;
+      b = cls.get_val_set_2(a);
+      `checkh(a, 2);
+      `checkh(b, 5);
+
+      mi = new(1);
+      b = cls.get_val_set_2(mi.x);
+      `checkh(mi.x, 2);
+      `checkh(b, 1);
+
+      arr[0] = 10;
+      b = cls.get_val_set_2(arr[0]);
+      `checkh(arr[0], 2);
+      `checkh(b, 10);
+
+      $write("*-* All Finished *-*\n");
+      $finish;
+   end
+endmodule

test_regress/t/t_func_ref_bad.out

@@ -0,0 +1,4 @@
+%Error: t/t_func_ref_bad.v:19:22: Function/task ref argument is not of allowed type
+   19 |       b = cls.get_x(a[1]);
+      |                      ^
+%Error: Exiting due to

test_regress/t/t_func_ref_bad.pl

@@ -0,0 +1,19 @@
+#!/usr/bin/perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2003 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 => 1);
+
+lint(
+    fails => 1,
+    expect_filename => $Self->{golden_filename},
+    );
+
+ok(1);
+1;

test_regress/t/t_func_ref_bad.v

@@ -0,0 +1,22 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Antmicro Ltd.
+// SPDX-License-Identifier: CC0-1.0
+
+class Cls;
+   function logic get_x(ref logic x);
+      return x;
+   endfunction
+endclass
+
+module t (/*AUTOARG*/);
+   logic [10:0] a;
+   logic b;
+   Cls cls;
+   initial begin
+      cls = new;
+      b = cls.get_x(a[1]);
+      $stop;
+   end
+endmodule

test_regress/t/t_func_ref_unsup.out

@@ -0,0 +1,8 @@
+%Error-TASKNSVAR: t/t_func_ref_unsup.v:29:28: Unsupported: ref argument of inlined function/task is not a simple variable
+   29 |       b = get_val_set_5(mi.x);
+      |                            ^
+                  ... For error description see https://verilator.org/warn/TASKNSVAR?v=latest
+%Error-TASKNSVAR: t/t_func_ref_unsup.v:34:28: Unsupported: ref argument of inlined function/task is not a simple variable
+   34 |       b = get_val_set_5(arr[0]);
+      |                            ^
+%Error: Exiting due to

test_regress/t/t_func_ref_unsup.pl

@@ -0,0 +1,19 @@
+#!/usr/bin/perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2003 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 => 1);
+
+lint(
+    fails => 1,
+    expect_filename => $Self->{golden_filename},
+    );
+
+ok(1);
+1;

test_regress/t/t_func_ref_unsup.v

@@ -0,0 +1,41 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Antmicro Ltd.
+// 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);
+
+class MyInt;
+   int x;
+   function new(int a);
+      x = a;
+   endfunction
+endclass
+
+function int get_val_set_5(ref int x);
+   automatic int y = x;
+   x = 5;
+   return y;
+endfunction
+
+module t (/*AUTOARG*/);
+   int b;
+   int arr[1];
+   MyInt mi;
+   initial begin
+      mi = new(1);
+      b = get_val_set_5(mi.x);
+      `checkh(mi.x, 5);
+      `checkh(b, 1);
+
+      arr[0] = 10;
+      b = get_val_set_5(arr[0]);
+      `checkh(arr[0], 5);
+      `checkh(b, 10);
+
+      $write("*-* All Finished *-*\n");
+      $finish;
+   end
+endmodule

test_regress/t/t_queue_persistence_inl_unsup.out

@@ -0,0 +1,5 @@
+%Error-TASKNSVAR: t/t_queue_persistence.v:30:13: Unsupported: ref argument of inlined function/task is not a simple variable
+   30 |       func(q[1]);
+      |             ^
+                  ... For error description see https://verilator.org/warn/TASKNSVAR?v=latest
+%Error: Exiting due to

test_regress/t/t_queue_persistence.pl → test_regress/t/t_queue_persistence_inl_unsup.pl

@@ -19,11 +19,8 @@
     compile(
         timing_loop => 1,
         verilator_flags2 => ["--timing"],
-        );
-
-    execute(
-        fails => $Self->{vlt_all},  # bug3385 need to fix "ref"
-        check_finished => !$Self->{vlt_all},
+        fails => 1,   # bug3385 need to fix "ref"
+        expect_filename => $Self->{golden_filename},
         );
 }
 

test_regress/t/t_queue_persistence_noinl.pl

@@ -22,8 +22,7 @@
         );
 
     execute(
-        fails => $Self->{vlt_all},  # bug3385 need to fix "ref"
-        check_finished => !$Self->{vlt_all},
+        check_finished => 1,
         );
 }