Re: [Patch][OpenMP][Fortran] Support absent optional args with use_device_{ptr, addr} (+ OpenACC's use_device clause)
Hi!
On 2019-11-11T13:14:43+0100, I wrote:
> On 2019-11-08T16:41:23+0100, Tobias Burnus wrote:
>> --- /dev/null
>> +++ b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
>
> When adding '{ dg-do run }' for torture testing (please remember to), I
> see the '-O0' and '-O1' execution testing FAIL, complaining that
> "libgomp: use_device_ptr pointer wasn't mapped".
That'd gotten resolved in the mean time, so I've now myself pushed to
master branch in commit b9dc11b6730a8030cfc85f0222cef523c9c5d27c "Torture
testing: 'libgomp.fortran/use_device_ptr-optional-2.f90'", see attached.
Grüße
Thomas
-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
>From b9dc11b6730a8030cfc85f0222cef523c9c5d27c Mon Sep 17 00:00:00 2001
From: Thomas Schwinge
Date: Mon, 11 Nov 2019 11:30:33 +0100
Subject: [PATCH] Torture testing:
'libgomp.fortran/use_device_ptr-optional-2.f90'
Fix-up for commit a2c26c50310a336361d8129ecdd43d3001d6cb3a (r278046) "Fortran]
Support absent optional args with use_device_{ptr,addr}".
libgomp/
* testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: Add
'dg-do run'.
---
libgomp/ChangeLog| 5 +
.../testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 | 1 +
2 files changed, 6 insertions(+)
diff --git a/libgomp/ChangeLog b/libgomp/ChangeLog
index ee1764d4ae31..53bb8d23fa15 100644
--- a/libgomp/ChangeLog
+++ b/libgomp/ChangeLog
@@ -1,3 +1,8 @@
+2020-04-29 Thomas Schwinge
+
+ * testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: Add
+ 'dg-do run'.
+
2020-04-23 Andrew Stubbs
PR other/94629
diff --git a/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
index 641ebd98962a..7a4aaae52cbd 100644
--- a/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
+++ b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
@@ -1,3 +1,4 @@
+! { dg-do run }
! Check whether absent optional arguments are properly
! handled with use_device_{addr,ptr}.
program main
--
2.26.2
Re: [Patch][OpenMP][Fortran] Support absent optional args with use_device_{ptr,addr} (+ OpenACC's use_device clause)
Hi Tobias!
Thanks for looking into this mess ;-) of Fortran optional arguments
support for OMP, based on what Kwok has already developed.
On 2019-11-08T16:41:23+0100, Tobias Burnus wrote:
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
When adding '{ dg-do run }' for torture testing (please remember to), I
see the '-O0' and '-O1' execution testing FAIL, complaining that
"libgomp: use_device_ptr pointer wasn't mapped".
Grüße
Thomas
> @@ -0,0 +1,33 @@
> +! Check whether absent optional arguments are properly
> +! handled with use_device_{addr,ptr}.
> +program main
> + implicit none (type, external)
> + call foo()
> +contains
> + subroutine foo(v, w, x, y, z)
> +integer, target, optional, value :: v
> +integer, target, optional :: w
> +integer, target, optional :: x(:)
> +integer, target, optional, allocatable :: y
> +integer, target, optional, allocatable :: z(:)
> +integer :: d
> +
> +!$omp target data map(d) use_device_addr(v, w, x, y, z)
> + if(present(v)) stop 1
> + if(present(w)) stop 2
> + if(present(x)) stop 3
> + if(present(y)) stop 4
> + if(present(z)) stop 5
> +!$omp end target data
> +
> +! Using 'v' in use_device_ptr gives an ICE
> +! TODO: Find out what the OpenMP spec permits for use_device_ptr
> +
> +!$omp target data map(d) use_device_ptr(w, x, y, z)
> + if(present(w)) stop 6
> + if(present(x)) stop 7
> + if(present(y)) stop 8
> + if(present(z)) stop 9
> +!$omp end target data
> + end subroutine foo
> +end program main
signature.asc
Description: PGP signature
Re: [Patch][OpenMP][Fortran] Support absent optional args with use_device_{ptr,addr} (+ OpenACC's use_device clause)
On Fri, Nov 08, 2019 at 04:41:23PM +0100, Tobias Burnus wrote:
> With DECL_ARTIFICIAL added and also_value replaced:
> Build on x86-64-gnu-linux. OK once regtested?
Almost.
> - gimplify_assign (x, var, );
> + if (do_optional_check && omp_check_optional_argument (ovar, true))
Do you need true here when just testing for non-NULL?
If yes, it would be better to call it just once, so that e.g. the
DECL_ARGUMENTS list is not walked twice. So perhaps:
tree present;
present = (do_optional_check
? omp_check_optional_argument (ovar, true) : NULL_TREE);
if (present)
{
> + {
> + tree null_label = create_artificial_label (UNKNOWN_LOCATION);
> + tree notnull_label = create_artificial_label (UNKNOWN_LOCATION);
> + tree opt_arg_label = create_artificial_label (UNKNOWN_LOCATION);
I this this is already too long, so needs line wrapping before =.
> + tree new_x = unshare_expr (x);
> + tree present = omp_check_optional_argument (ovar, true);
And not call it here again.
> + gimplify_expr (, , NULL, is_gimple_val,
> +fb_rvalue);
> + gcond *cond = gimple_build_cond_from_tree (present,
> +notnull_label,
> +null_label);
> + gimple_seq_add_stmt (, cond);
> + gimple_seq_add_stmt (, gimple_build_label (null_label));
> + gimplify_assign (new_x, null_pointer_node, );
> + gimple_seq_add_stmt (, gimple_build_goto (opt_arg_label));
And here similarly.
> + if (do_optional_check
> + && omp_check_optional_argument (OMP_CLAUSE_DECL (c), true))
> + {
> + tree null_label = create_artificial_label (UNKNOWN_LOCATION);
> + tree notnull_label = create_artificial_label (UNKNOWN_LOCATION);
> + tree opt_arg_label = create_artificial_label (UNKNOWN_LOCATION);
> + glabel *null_glabel = gimple_build_label (null_label);
> + glabel *notnull_glabel = gimple_build_label (notnull_label);
> + ggoto *opt_arg_ggoto = gimple_build_goto (opt_arg_label);
> + gimplify_expr (, _body, NULL, is_gimple_val,
> +fb_rvalue);
> + tree present = omp_check_optional_argument (OMP_CLAUSE_DECL (c),
> + true);
Similarly to the above.
Otherwise LGTM.
Jakub
Re: [Patch][OpenMP][Fortran] Support absent optional args with use_device_{ptr,addr} (+ OpenACC's use_device clause)
Hi Jakub, thanks for the review. On 11/8/19 3:39 PM, Jakub Jelinek wrote: + /* Walk function argument list to find the hidden arg. */ + decl = DECL_ARGUMENTS (DECL_CONTEXT (decl)); + for ( ; decl != NULL_TREE; decl = TREE_CHAIN (decl)) + if (DECL_NAME (decl) == tree_name) + break; Is this reliable? I mean, consider -fallow-leading-underscore with: subroutine foo (a, _a) I also assume that this will break; unlikely in real-world code but still. Not really sure if additional DECL_ARTIFICIAL (decl) test would be enough. At least, I cannot quickly come up with a case where it will break. – I have now added it; also to the existing trans-expr.c function – which uses the used and, hence, same algorithm. +omp_check_optional_argument (tree decl, bool also_value) Why is the argument called for_present_check in the langhook and also_value here? Looks inconsistent. Because I initially was thinking only of the VALUE attribute until I realized that assumed-shape arrays have the same issue; they use a local variable for the data – and make the actual array descriptor available via lang-specific field. – As the use is either an extra deref is needed or a check whether the variable is present, I changed the meaning – seemingly, three places survived with the old name. With DECL_ARTIFICIAL added and also_value replaced: Build on x86-64-gnu-linux. OK once regtested? Tobias 2019-11-08 Tobias Burnus Kwok Cheung Yeung gcc/ * langhooks-def.h (LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT): Renamed from LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT; update define. (LANG_HOOKS_DECLS): Rename also here. * langhooks.h (lang_hooks_for_decls): Rename omp_is_optional_argument to omp_check_optional_argument; take additional bool argument. * omp-general.h (omp_check_optional_argument): Likewise. * omp-general.h (omp_check_optional_argument): Likewise. * omp-low.c (lower_omp_target): Update calls; handle absent Fortran optional arguments with USE_DEVICE_ADDR/USE_DEVICE_PTR. gcc/fortran/ * trans-expr.c (gfc_conv_expr_present): Check for DECL_ARTIFICIAL for the VALUE hidden argument avoiding -fallow-underscore issues. * trans-decl.c (create_function_arglist): Also set GFC_DECL_OPTIONAL_ARGUMENT for per-value arguments. * f95-lang.c (LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT): Renamed from LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT; point to gfc_omp_check_optional_argument. * trans.h (gfc_omp_check_optional_argument): Subsitutes gfc_omp_is_optional_argument declaration. * trans-openmp.c (gfc_omp_is_optional_argument): Make static. (gfc_omp_check_optional_argument): New function. libgomp/ * testsuite/libgomp.fortran/use_device_ptr-optional-1.f90: Extend. * testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: New. gcc/fortran/f95-lang.c | 4 +- gcc/fortran/trans-decl.c | 3 +- gcc/fortran/trans-expr.c | 3 +- gcc/fortran/trans-openmp.c | 63 ++- gcc/fortran/trans.h| 2 +- gcc/langhooks-def.h| 4 +- gcc/langhooks.h| 13 ++- gcc/omp-general.c | 14 ++- gcc/omp-general.h | 2 +- gcc/omp-low.c | 117 - .../libgomp.fortran/use_device_ptr-optional-1.f90 | 22 .../libgomp.fortran/use_device_ptr-optional-2.f90 | 33 ++ 12 files changed, 232 insertions(+), 48 deletions(-) diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c index 0684c3b99cf..c7b592dbfe2 100644 --- a/gcc/fortran/f95-lang.c +++ b/gcc/fortran/f95-lang.c @@ -115,7 +115,7 @@ static const struct attribute_spec gfc_attribute_table[] = #undef LANG_HOOKS_INIT_TS #undef LANG_HOOKS_OMP_ARRAY_DATA #undef LANG_HOOKS_OMP_IS_ALLOCATABLE_OR_PTR -#undef LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT +#undef LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT #undef LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE #undef LANG_HOOKS_OMP_PREDETERMINED_SHARING #undef LANG_HOOKS_OMP_REPORT_DECL @@ -150,7 +150,7 @@ static const struct attribute_spec gfc_attribute_table[] = #define LANG_HOOKS_INIT_TS gfc_init_ts #define LANG_HOOKS_OMP_ARRAY_DATA gfc_omp_array_data #define LANG_HOOKS_OMP_IS_ALLOCATABLE_OR_PTR gfc_omp_is_allocatable_or_ptr -#define LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT gfc_omp_is_optional_argument +#define LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT gfc_omp_check_optional_argument #define LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE gfc_omp_privatize_by_reference #define LANG_HOOKS_OMP_PREDETERMINED_SHARING gfc_omp_predetermined_sharing #define LANG_HOOKS_OMP_REPORT_DECL gfc_omp_report_decl diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index ffa6316..80ef45d892e 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -2691,9 +2691,8 @@
Re: [Patch][OpenMP][Fortran] Support absent optional args with use_device_{ptr,addr} (+ OpenACC's use_device clause)
On Thu, Nov 07, 2019 at 11:42:22AM +0100, Tobias Burnus wrote:
> + /* For VALUE, the scalar variable is passed as is but a hidden argument
> + denotes the value. Cf. trans-expr.c. */
> + if (TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE)
> +{
> + char name[GFC_MAX_SYMBOL_LEN + 2];
> + tree tree_name;
> +
> + name[0] = '_';
> + strcpy ([1], IDENTIFIER_POINTER (DECL_NAME (decl)));
> + tree_name = get_identifier (name);
> +
> + /* Walk function argument list to find the hidden arg. */
> + decl = DECL_ARGUMENTS (DECL_CONTEXT (decl));
> + for ( ; decl != NULL_TREE; decl = TREE_CHAIN (decl))
> + if (DECL_NAME (decl) == tree_name)
> + break;
> +
> + gcc_assert (decl);
> + return decl;
> +}
Is this reliable? I mean, consider -fallow-leading-underscore with:
subroutine foo (a, _a)
integer, optional, value :: a
logical(kind=1), value :: _a
...
end subroutine foo
and whatever OpenMP clause is affected in ...
In GIMPLE dump I certainly see:
foo (integer(kind=4) a, logical(kind=1) _a, logical(kind=1) _a)
and I bet the above would pick the wrong one.
Not really sure if additional DECL_ARTIFICIAL (decl) test would be enough.
> --- a/gcc/omp-general.c
> +++ b/gcc/omp-general.c
> @@ -63,12 +63,18 @@ omp_is_allocatable_or_ptr (tree decl)
>return lang_hooks.decls.omp_is_allocatable_or_ptr (decl);
> }
>
> -/* Return true if DECL is a Fortran optional argument. */
> +/* Check whether this DECL belongs to a Fortran optional argument.
> + With 'for_present_check' set to false, decls which are optional parameters
> + themselve are returned as tree - or a NULL_TREE otherwise. Those decls are
> + always pointers. With 'for_present_check' set to true, the decl for
> checking
> + whether an argument is present is returned; for arguments with value
> + attribute this is the hidden argument and of BOOLEAN_TYPE. If the decl is
> + unrelated to optional arguments, NULL_TREE is returned. */
>
> -bool
> -omp_is_optional_argument (tree decl)
> +tree
> +omp_check_optional_argument (tree decl, bool also_value)
Why is the argument called for_present_check in the langhook and
also_value here? Looks inconsistent.
> --- a/gcc/omp-general.h
> +++ b/gcc/omp-general.h
> @@ -74,7 +74,7 @@ struct omp_for_data
>
> extern tree omp_find_clause (tree clauses, enum omp_clause_code kind);
> extern bool omp_is_allocatable_or_ptr (tree decl);
> -extern bool omp_is_optional_argument (tree decl);
> +extern tree omp_check_optional_argument (tree decl, bool also_value);
> extern bool omp_is_reference (tree decl);
> extern void omp_adjust_for_condition (location_t loc, enum tree_code
> *cond_code,
> tree *n2, tree v, tree step);
Here too.
Jakub
Re: [Patch][OpenMP][Fortran] Support absent optional args with use_device_{ptr,addr} (+ OpenACC's use_device clause)
Thinking about the patch over night, I have now updated it a bit:
Namely, I only add the "if(present-check)" condition, if the original
variable is dereferenced. There is no need for code like
omp_data_arr.c = c == NULL ? NULL : c;
and then, after the libgomp call, code like "c_2 = c == NULL ? NULL :
omp_data_arr.c;"; due to the libgomp call, the latter cannot even be
optimized away.
Hence, I added 'do_optional_check'; additionally, I had a
libgomp.fortran/use_device_ptr-optional-1.f90 change floating around,
which I included. Otherwise unchanged.
Retested. OK for the trunk?
Cheers,
Tobias
On 11/6/19 4:04 PM, Tobias Burnus wrote:
This patch is based on Kwok's patch, posted as (4/5) at
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00964.html – which is
targeting OpenACC's use_device* – but it also applies to OpenMP
use_device_{ptr,addr}.
I added an OpenMP test case. It showed that for arguments with value
attribute and for assumed-shape array, one needs to do more — as the
decl cannot be directly used for the is-argument-present check.
(For 'value', a hidden boolean '_' + arg-name is passed in addition;
for assumed-shape arrays, the array descriptor "x" is replaced by the
local variable "x.0" (with "x.0 = x->data") and the original decl "x"
is in GFC_DECL_SAVED_DESCRIPTOR. Especially for assumed-shape arrays,
the new decl cannot be used unconditionally as it is uninitialized
when the argument is absent.)
Bootstrapped and regtested on x86_64-gnu-linux without offloading +
with nvptx.
OK?
Cheers,
Tobias
*The OpenACC test cases are in 5/5 and depend on some other changes.
Submission of {1,missing one line of 2,3,5}/5 is planned next.
PPS: For fully absent-optional support, mapping needs to be handled
for OpenACC (see Kwok's …/5 patches) and OpenMP (which is quite
different on FE level) – and OpenMP also needs changes for the share
clauses.]
2019-11-07 Tobias Burnus
Kwok Cheung Yeung
gcc/
* langhooks-def.h (LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT):
Renamed from LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT; update define.
(LANG_HOOKS_DECLS): Rename also here.
* langhooks.h (lang_hooks_for_decls): Rename
omp_is_optional_argument to omp_check_optional_argument; take
additional bool argument.
* omp-general.h (omp_check_optional_argument): Likewise.
* omp-general.h (omp_check_optional_argument): Likewise.
* omp-low.c (lower_omp_target): Update calls; handle absent
Fortran optional arguments with USE_DEVICE_ADDR/USE_DEVICE_PTR.
gcc/fortran/
* trans-decl.c (create_function_arglist): Also set
GFC_DECL_OPTIONAL_ARGUMENT for per-value arguments.
* f95-lang.c (LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT):
Renamed from LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT; point
to gfc_omp_check_optional_argument.
* trans.h (gfc_omp_check_optional_argument): Subsitutes
gfc_omp_is_optional_argument declaration.
* trans-openmp.c (gfc_omp_is_optional_argument): Make static.
(gfc_omp_check_optional_argument): New function.
libgomp/
* testsuite/libgomp.fortran/use_device_ptr-optional-1.f90: Extend.
* testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: New.
gcc/fortran/f95-lang.c | 4 ++--
gcc/fortran/trans-decl.c| 3 +--
gcc/fortran/trans-openmp.c | 62 +-
gcc/fortran/trans.h | 2 +-
gcc/langhooks-def.h | 4 ++--
gcc/langhooks.h | 13 -
gcc/omp-general.c | 14 ++
gcc/omp-general.h | 2 +-
gcc/omp-low.c | 117 -
libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-1.f90 | 22 ++
libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90 | 33 +
11 files changed, 229 insertions(+), 47 deletions(-)
diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
index 0684c3b99cf..c7b592dbfe2 100644
--- a/gcc/fortran/f95-lang.c
+++ b/gcc/fortran/f95-lang.c
@@ -115,7 +115,7 @@ static const struct attribute_spec gfc_attribute_table[] =
#undef LANG_HOOKS_INIT_TS
#undef LANG_HOOKS_OMP_ARRAY_DATA
#undef LANG_HOOKS_OMP_IS_ALLOCATABLE_OR_PTR
-#undef LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT
+#undef LANG_HOOKS_OMP_CHECK_OPTIONAL_ARGUMENT
#undef LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE
#undef LANG_HOOKS_OMP_PREDETERMINED_SHARING
#undef LANG_HOOKS_OMP_REPORT_DECL
@@ -150,7 +150,7 @@ static const struct attribute_spec gfc_attribute_table[] =
#define LANG_HOOKS_INIT_TS gfc_init_ts
#define LANG_HOOKS_OMP_ARRAY_DATA
