* [Patch][OpenMP] Fix use_device_… with absent optional arg
@ 2019-11-21 16:59 Tobias Burnus
2019-11-29 12:07 ` Tobias Burnus
0 siblings, 1 reply; 4+ messages in thread
From: Tobias Burnus @ 2019-11-21 16:59 UTC (permalink / raw)
To: gcc-patches, fortran, Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]
This fixes two issues with the recently added absent-optional patch for
use_device_â¦
(a) For nonallocatable, nonpointer arrays the data component of the
array descriptor is replaced by a local variable â if the argument is
absent, this variable is not initialized and, unless it is NULL, a
pointer to undefined memory attempted to be mapped.
(b) For per-value arguments, the dummy argument itself always exists and
as it is not a pointer, there is nothing to dereference. Hence,
'use_device_addr(val_arg)' can be run unconditionally. However, doing so
will fail if 'val_arg' has never been mapped to the device. â I think
the most sensible is to update the test case. (One could add a
condition, to use a NULL pointer if absent, but as I cannot come up with
a valid program, leaving the condition out in the generated code makes
more sense.)
OK?
Tobias
PS: I wonder why I didn't see it when initially submitting the patch. I
think it must be after I did a small change and did a quick regtesting.
I assume for some reasons the device became unavailable â turning all
checks into unsupported and I only looked for FAIL and didn't check
whether some new unsupported popped up. â Namely,
use_device_addr-{3,4}.f90 failed without (a) â but only with -O1 (and
with 1 of 11 (hardware, cuda version) combos with -Os).
use_device_ptr-optional-2.f90 failed with '-O' (which is the only option
which was actually run) due to (a) and (b).
[-- Attachment #2: fix-omp-optional.diff --]
[-- Type: text/x-patch, Size: 1840 bytes --]
gcc/
* omp-low.c (lower_omp_target): Also add is-present condition for
nonallocatable, nonpointer, optional arguments.
libgomp/
* testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: Map
per-value optional arg before using it in use_device_addr.
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 19132f76da2..94f830c644b 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -12029,6 +12029,8 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
var = build_simple_mem_ref (var);
do_optional_check = true;
}
+ else if (TREE_CODE (type) == ARRAY_TYPE)
+ do_optional_check = true;
var = fold_convert (TREE_TYPE (x), var);
}
}
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 41abf17eede..bad0e00a23a 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,5 @@
+! {dg-do run }
+!
! Check whether absent optional arguments are properly
! handled with use_device_{addr,ptr}.
program main
@@ -12,6 +14,10 @@ contains
integer, target, optional, allocatable :: z(:)
integer :: d
+ !$omp target data map(to:v)
+ ! If 'v' should be usable as device pointer, it has to be mapped at some
+ ! point. As it is declared with the VALUE attribute, it needs to be done
+ ! within 'foo'
!$omp target data map(d) use_device_addr(v, w, x, y, z)
if(present(v)) stop 1
if(present(w)) stop 2
@@ -19,6 +25,7 @@ contains
if(present(y)) stop 4
if(present(z)) stop 5
!$omp end target data
+ !$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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch][OpenMP] Fix use_device_… with absent optional arg
2019-11-21 16:59 [Patch][OpenMP] Fix use_device_… with absent optional arg Tobias Burnus
@ 2019-11-29 12:07 ` Tobias Burnus
2019-12-05 11:46 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Tobias Burnus @ 2019-11-29 12:07 UTC (permalink / raw)
To: gcc-patches, fortran, Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]
Revised patch after some re-considerations (and finding tons of issues
with VALUE + OPTIONAL in gfortran itself). â What the patch does [all
related to use_device_{addr,ptr} and 'optional' arguments]:
For assumed-shape arrays, the compiler generates code like "if (arg)
arg.0 = arg->data;". Hence, arg.0 was always available â but possibly
pointing to uninitialized memory. â Likewise, per-value-passed
arguments, 'arg' (i.e. &arg) is always available. â But, in the absent
case, if 'arg->data is not NULL or the per-value decl is not mapped (cf.
test case), that's not the best idea.
I thought that I don't need a condition in thoses case â but it turns
out that the offloading plugin might (rightly!) complain that the
address is not mapped. â Hence, I add the is-present condition now also
in for those case; as none remain, the do_optional_check is now gone.
However, after the library call, amp_arr.arg is known to be initialized.
In this case, I keep the do_optional_check check. â This avoids code
which boils down to "x0 = arg ? omp_arr.arg : NULL". â and keeps
generating condtions only for complex code such as:Â if (present) {
tmp.data = omp_arr.arg; arg = &tmp; } else {arg = NULL;}.
Finally, while testing/exploring value+optional bugs, I stumbled over
'type(c_ptr),value' which is 'void *'. In particular, it is pointer but
still the is-present status is passed as hidden argument. This patch
fixes both mapping and the is-present check.
Build on x86-64-gnu-linux + tested once on a system without offloading
support and with nvptx offloading.
OK?
Tobias
PS: Regarding the issues with OPTIONAL and VALUE, see PR fortran/92703
and PR fortran/92702. Found issues: const-len character strings are
passed as value w/o hidden is-present arg but func call passes them;
those and derived types/the outer class container are passed by value -
but the 'present()' check assumes pointers (hence: ICE); if basent
null_ptr_node is passed, I fear that this will give stack issues, at
least on some platforms. â Additionally, deferred-length character
strings and arrays are permitted since F2008 but not yet supported.
[-- Attachment #2: optional-fix-v2.diff --]
[-- Type: text/x-patch, Size: 6382 bytes --]
gcc/fortran/
* trans-openmp.c (gfc_omp_is_optional_argument,
gfc_omp_check_optional_argument): Handle type(c_ptr),value which uses a
hidden argument for the is-present check.
gcc/
* omp-low.c (lower_omp_target): For use_device_ptr/use_derice_addr
and Fortran's optional arguments, unconditionally add the is-present
condition before the libgomp call.
libgomp/
* testsuite/libgomp.fortran/use_device_ptr-optional-2.f90: Add
'type(c_ptr), value' test case. Conditionally map the per-value
passed arguments.
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
index d9dfcabc65e..f21785fa8c3 100644
--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -60,7 +60,8 @@ gfc_omp_is_allocatable_or_ptr (const_tree decl)
/* True if the argument is an optional argument; except that false is also
returned for arguments with the value attribute (nonpointers) and for
- assumed-shape variables (decl is a local variable containing arg->data). */
+ assumed-shape variables (decl is a local variable containing arg->data).
+ Note that pvoid_type_node is for 'type(c_ptr), value. */
static bool
gfc_omp_is_optional_argument (const_tree decl)
@@ -68,6 +69,7 @@ gfc_omp_is_optional_argument (const_tree decl)
return (TREE_CODE (decl) == PARM_DECL
&& DECL_LANG_SPECIFIC (decl)
&& TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE
+ && TREE_TYPE (decl) != pvoid_type_node
&& GFC_DECL_OPTIONAL_ARGUMENT (decl));
}
@@ -99,9 +101,12 @@ gfc_omp_check_optional_argument (tree decl, bool for_present_check)
|| !GFC_DECL_OPTIONAL_ARGUMENT (decl))
return NULL_TREE;
- /* 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)
+ /* Scalars with VALUE attribute which are passed by value use a hidden
+ argument to denote the present status. They are passed as nonpointer type
+ with one exception: 'type(c_ptr), value' as '*void'. */
+ /* Cf. trans-expr.c's gfc_conv_expr_present. */
+ if (TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE
+ || TREE_TYPE (decl) == pvoid_type_node)
{
char name[GFC_MAX_SYMBOL_LEN + 2];
tree tree_name;
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 19132f76da2..0e66a68ff36 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11981,8 +11981,6 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
case OMP_CLAUSE_USE_DEVICE_PTR:
case OMP_CLAUSE_USE_DEVICE_ADDR:
case OMP_CLAUSE_IS_DEVICE_PTR:
- bool do_optional_check;
- do_optional_check = false;
ovar = OMP_CLAUSE_DECL (c);
var = lookup_decl_in_outer_ctx (ovar, ctx);
@@ -12004,10 +12002,7 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
}
type = TREE_TYPE (ovar);
if (lang_hooks.decls.omp_array_data (ovar, true))
- {
- var = lang_hooks.decls.omp_array_data (ovar, false);
- do_optional_check = true;
- }
+ var = lang_hooks.decls.omp_array_data (ovar, false);
else if ((OMP_CLAUSE_CODE (c) == OMP_CLAUSE_USE_DEVICE_ADDR
&& !omp_is_reference (ovar)
&& !omp_is_allocatable_or_ptr (ovar))
@@ -12027,14 +12022,12 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
&& omp_is_allocatable_or_ptr (ovar))))
{
var = build_simple_mem_ref (var);
- do_optional_check = true;
}
var = fold_convert (TREE_TYPE (x), var);
}
}
tree present;
- present = (do_optional_check
- ? omp_check_optional_argument (ovar, true) : NULL_TREE);
+ present = omp_check_optional_argument (ovar, true);
if (present)
{
tree null_label = create_artificial_label (UNKNOWN_LOCATION);
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 41abf17eede..e57abfbed5c 100644
--- a/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
+++ b/libgomp/testsuite/libgomp.fortran/use_device_ptr-optional-2.f90
@@ -1,33 +1,47 @@
! Check whether absent optional arguments are properly
! handled with use_device_{addr,ptr}.
program main
+ use iso_c_binding, only: c_ptr, c_loc
implicit none (type, external)
call foo()
contains
- subroutine foo(v, w, x, y, z)
+ subroutine foo(v, w, x, y, z, cptr)
integer, target, optional, value :: v
integer, target, optional :: w
integer, target, optional :: x(:)
integer, target, optional, allocatable :: y
integer, target, optional, allocatable :: z(:)
+ type(c_ptr), target, optional, value :: cptr
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
+ ! Need to map per-VALUE arguments, if present
+ if (present(v)) then
+ !$omp target enter data map(to:v)
+ stop 1 ! â but it shall not be present in this test case.
+ end if
+ if (present(cptr)) then
+ !$omp target enter data map(to:cptr)
+ stop 2 ! â but it shall not be present in this test case.
+ end if
+
+ !$omp target data map(d) use_device_addr(v, w, x, y, z, cptr)
+ if (present(v)) then; v = 5; stop 1; endif
+ if (present(w)) then; w = 5; stop 2; endif
+ if (present(x)) then; x(1) = 5; stop 3; endif
+ if (present(y)) then; y = 5; stop 4; endif
+ if (present(z)) then; z(1) = 5; stop 5; endif
+ if (present(cptr)) then; cptr = c_loc(v); stop 6; endif
!$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 target data map(d) use_device_ptr(w, x, y, z, cptr)
+ if(present(w)) then; w = 5; stop 11; endif
+ if(present(x)) then; x(1) = 5; stop 12; endif
+ if(present(y)) then; y = 5; stop 13; endif
+ if(present(z)) then; z(1) = 5; stop 14; endif
+ if(present(cptr)) then; cptr = c_loc(x); stop 15; endif
!$omp end target data
end subroutine foo
end program main
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch][OpenMP] Fix use_device_… with absent optional arg
2019-11-29 12:07 ` Tobias Burnus
@ 2019-12-05 11:46 ` Jakub Jelinek
2019-12-05 15:20 ` Tobias Burnus
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2019-12-05 11:46 UTC (permalink / raw)
To: Tobias Burnus; +Cc: gcc-patches, fortran
On Fri, Nov 29, 2019 at 01:03:13PM +0100, Tobias Burnus wrote:
> --- a/gcc/fortran/trans-openmp.c
> +++ b/gcc/fortran/trans-openmp.c
> @@ -60,7 +60,8 @@ gfc_omp_is_allocatable_or_ptr (const_tree decl)
>
> /* True if the argument is an optional argument; except that false is also
> returned for arguments with the value attribute (nonpointers) and for
> - assumed-shape variables (decl is a local variable containing arg->data). */
> + assumed-shape variables (decl is a local variable containing arg->data).
> + Note that pvoid_type_node is for 'type(c_ptr), value. */
>
> static bool
> gfc_omp_is_optional_argument (const_tree decl)
> @@ -68,6 +69,7 @@ gfc_omp_is_optional_argument (const_tree decl)
> return (TREE_CODE (decl) == PARM_DECL
> && DECL_LANG_SPECIFIC (decl)
> && TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE
> + && TREE_TYPE (decl) != pvoid_type_node
Is it always pvoid_type_node, or could it be say const qualified version
thereof etc. (C void const *) etc.? If the latter, then
&& !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (decl)))
might be better check. If it is always just pvoid_type_node, the above is
fine sure.
> && GFC_DECL_OPTIONAL_ARGUMENT (decl));
> }
>
> @@ -99,9 +101,12 @@ gfc_omp_check_optional_argument (tree decl, bool for_present_check)
> || !GFC_DECL_OPTIONAL_ARGUMENT (decl))
> return NULL_TREE;
>
> - /* 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)
> + /* Scalars with VALUE attribute which are passed by value use a hidden
> + argument to denote the present status. They are passed as nonpointer type
> + with one exception: 'type(c_ptr), value' as '*void'. */
void* or void * instead of *void?
> + /* Cf. trans-expr.c's gfc_conv_expr_present. */
> + if (TREE_CODE (TREE_TYPE (decl)) != POINTER_TYPE
> + || TREE_TYPE (decl) == pvoid_type_node)
And similar question to the above one.
> @@ -12027,14 +12022,12 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, omp_context *ctx)
> && omp_is_allocatable_or_ptr (ovar))))
> {
> var = build_simple_mem_ref (var);
> - do_optional_check = true;
> }
The above then becomes { single_stmt } and so should be changed to
single_stmt without {}s around it.
> @@ -1,33 +1,47 @@
> ! Check whether absent optional arguments are properly
> ! handled with use_device_{addr,ptr}.
> program main
> + use iso_c_binding, only: c_ptr, c_loc
> implicit none (type, external)
> call foo()
> contains
> - subroutine foo(v, w, x, y, z)
> + subroutine foo(v, w, x, y, z, cptr)
> integer, target, optional, value :: v
> integer, target, optional :: w
> integer, target, optional :: x(:)
> integer, target, optional, allocatable :: y
> integer, target, optional, allocatable :: z(:)
> + type(c_ptr), target, optional, value :: cptr
> 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
> + ! Need to map per-VALUE arguments, if present
> + if (present(v)) then
> + !$omp target enter data map(to:v)
> + stop 1 ! – but it shall not be present in this test case.
> + end if
> + if (present(cptr)) then
> + !$omp target enter data map(to:cptr)
> + stop 2 ! – but it shall not be present in this test case.
> + end if
Shouln't the stop arguments differ from anything else already in the
testcase?
> +
> + !$omp target data map(d) use_device_addr(v, w, x, y, z, cptr)
> + if (present(v)) then; v = 5; stop 1; endif
> + if (present(w)) then; w = 5; stop 2; endif
> + if (present(x)) then; x(1) = 5; stop 3; endif
> + if (present(y)) then; y = 5; stop 4; endif
> + if (present(z)) then; z(1) = 5; stop 5; endif
> + if (present(cptr)) then; cptr = c_loc(v); stop 6; endif
> !$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 target data map(d) use_device_ptr(w, x, y, z, cptr)
> + if(present(w)) then; w = 5; stop 11; endif
> + if(present(x)) then; x(1) = 5; stop 12; endif
> + if(present(y)) then; y = 5; stop 13; endif
> + if(present(z)) then; z(1) = 5; stop 14; endif
> + if(present(cptr)) then; cptr = c_loc(x); stop 15; endif
> !$omp end target data
> end subroutine foo
> end program main
Otherwise LGTM.
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch][OpenMP] Fix use_device_… with absent optional arg
2019-12-05 11:46 ` Jakub Jelinek
@ 2019-12-05 15:20 ` Tobias Burnus
0 siblings, 0 replies; 4+ messages in thread
From: Tobias Burnus @ 2019-12-05 15:20 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, fortran
Hi Jakub,
thanks for the review â committed as Rev. 279004.
On 12/5/19 12:46 PM, Jakub Jelinek wrote:
> On Fri, Nov 29, 2019 at 01:03:13PM +0100, Tobias Burnus wrote:
>> --- a/gcc/fortran/trans-openmp.c
>> + && TREE_TYPE (decl) != pvoid_type_node
> Is it always pvoid_type_node, or could it be say const qualified version
> thereof etc. (C void const *) etc.? If the latter, then
> && !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (decl)))
> might be better check. If it is always just pvoid_type_node, the above is
> fine sure.
I will use your version (twice) just to be sure. â Currently, I think it
will always be pvoid_type_node but I don't want to rule out some const
qualifier might be possible. (volatile is not permitted with value,
restricted and atomic qualifiers are unlikely.)
[â¦]
> Shouln't the stop arguments differ from anything else already in the
> testcase?
Yes â it makes failure debugging easier. I thought, I had re-enumerated,
but I missed the two.
Tobias
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-05 15:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-21 16:59 [Patch][OpenMP] Fix use_device_… with absent optional arg Tobias Burnus
2019-11-29 12:07 ` Tobias Burnus
2019-12-05 11:46 ` Jakub Jelinek
2019-12-05 15:20 ` Tobias Burnus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).