From 4636e9cffb709de7d953389b708854a6077f5149 Mon Sep 17 00:00:00 2001 From: Ryszard Rozak Date: Wed, 20 Sep 2023 13:33:11 +0200 Subject: [PATCH] Fix passing arguments by reference (#3385 partial) (#4489) --- src/V3Ast.h | 2 +- src/V3AstNodeOther.h | 1 + src/V3AstNodes.cpp | 5 +- src/V3LinkLevel.cpp | 2 +- src/V3Task.cpp | 36 +++++++++-- test_regress/t/t_func_inout_bit_sel.pl | 21 +++++++ test_regress/t/t_func_inout_bit_sel.v | 31 ++++++++++ test_regress/t/t_func_ref_arg.pl | 21 +++++++ test_regress/t/t_func_ref_arg.v | 60 +++++++++++++++++++ test_regress/t/t_func_ref_bad.out | 4 ++ test_regress/t/t_func_ref_bad.pl | 19 ++++++ test_regress/t/t_func_ref_bad.v | 22 +++++++ test_regress/t/t_func_ref_unsup.out | 8 +++ test_regress/t/t_func_ref_unsup.pl | 19 ++++++ test_regress/t/t_func_ref_unsup.v | 41 +++++++++++++ .../t/t_queue_persistence_inl_unsup.out | 5 ++ ...ce.pl => t_queue_persistence_inl_unsup.pl} | 7 +-- test_regress/t/t_queue_persistence_noinl.pl | 3 +- 18 files changed, 292 insertions(+), 15 deletions(-) create mode 100755 test_regress/t/t_func_inout_bit_sel.pl create mode 100644 test_regress/t/t_func_inout_bit_sel.v create mode 100755 test_regress/t/t_func_ref_arg.pl create mode 100644 test_regress/t/t_func_ref_arg.v create mode 100644 test_regress/t/t_func_ref_bad.out create mode 100755 test_regress/t/t_func_ref_bad.pl create mode 100644 test_regress/t/t_func_ref_bad.v create mode 100644 test_regress/t/t_func_ref_unsup.out create mode 100755 test_regress/t/t_func_ref_unsup.pl create mode 100644 test_regress/t/t_func_ref_unsup.v create mode 100644 test_regress/t/t_queue_persistence_inl_unsup.out rename test_regress/t/{t_queue_persistence.pl => t_queue_persistence_inl_unsup.pl} (84%) diff --git a/src/V3Ast.h b/src/V3Ast.h index 8c42e41ba..22e4fd846 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -706,7 +706,7 @@ public: 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; diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index 8d4622164..12b7ce9f2 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -1931,6 +1931,7 @@ public: 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; } diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index d28692165..81dfdbaf4 100644 --- a/src/V3AstNodes.cpp +++ b/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 diff --git a/src/V3LinkLevel.cpp b/src/V3LinkLevel.cpp index 1b0dc54cf..03ba5bf96 100644 --- a/src/V3LinkLevel.cpp +++ b/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()); diff --git a/src/V3Task.cpp b/src/V3Task.cpp index 246748d59..7586795a0 100644 --- a/src/V3Task.cpp +++ b/src/V3Task.cpp @@ -479,10 +479,38 @@ private: 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); diff --git a/test_regress/t/t_func_inout_bit_sel.pl b/test_regress/t/t_func_inout_bit_sel.pl new file mode 100755 index 000000000..beaa2ca59 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_func_inout_bit_sel.v b/test_regress/t/t_func_inout_bit_sel.v new file mode 100644 index 000000000..3af758d75 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_func_ref_arg.pl b/test_regress/t/t_func_ref_arg.pl new file mode 100755 index 000000000..aabcde63e --- /dev/null +++ b/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; diff --git a/test_regress/t/t_func_ref_arg.v b/test_regress/t/t_func_ref_arg.v new file mode 100644 index 000000000..b552a2b24 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_func_ref_bad.out b/test_regress/t/t_func_ref_bad.out new file mode 100644 index 000000000..aa39be63e --- /dev/null +++ b/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 diff --git a/test_regress/t/t_func_ref_bad.pl b/test_regress/t/t_func_ref_bad.pl new file mode 100755 index 000000000..23eda8f99 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_func_ref_bad.v b/test_regress/t/t_func_ref_bad.v new file mode 100644 index 000000000..22154bb05 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_func_ref_unsup.out b/test_regress/t/t_func_ref_unsup.out new file mode 100644 index 000000000..1888aea08 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_func_ref_unsup.pl b/test_regress/t/t_func_ref_unsup.pl new file mode 100755 index 000000000..23eda8f99 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_func_ref_unsup.v b/test_regress/t/t_func_ref_unsup.v new file mode 100644 index 000000000..6fa1e9d87 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_queue_persistence_inl_unsup.out b/test_regress/t/t_queue_persistence_inl_unsup.out new file mode 100644 index 000000000..d9e34ab83 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_queue_persistence.pl b/test_regress/t/t_queue_persistence_inl_unsup.pl similarity index 84% rename from test_regress/t/t_queue_persistence.pl rename to test_regress/t/t_queue_persistence_inl_unsup.pl index c9515ce9b..8cf7fc5b6 100755 --- a/test_regress/t/t_queue_persistence.pl +++ b/test_regress/t/t_queue_persistence_inl_unsup.pl @@ -19,11 +19,8 @@ else { 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}, ); } diff --git a/test_regress/t/t_queue_persistence_noinl.pl b/test_regress/t/t_queue_persistence_noinl.pl index afaa948eb..187562462 100755 --- a/test_regress/t/t_queue_persistence_noinl.pl +++ b/test_regress/t/t_queue_persistence_noinl.pl @@ -22,8 +22,7 @@ else { ); execute( - fails => $Self->{vlt_all}, # bug3385 need to fix "ref" - check_finished => !$Self->{vlt_all}, + check_finished => 1, ); }