Message ID | f335efac-abfa-f43e-2afc-fd1931740362@codesourcery.com |
---|---|
State | New |
Headers | show |
Series | Add support for Fortran optional arguments in OpenACC | expand |
Hi Tobias! Reviewing your <http://mid.mail-archive.com/8be82276-81b1-817c-fcd2-51f24f5fe2d2@codesourcery.com> "[Patch][OpenMP/OpenACC/Fortran] Fix mapping of optional (present|absent) arguments" reminded me that still this behavioral change has not been split out, cited below, that you described as "trivial". I've just filed <https://gcc.gnu.org/PR92726> "OpenACC: 'NULL'-in -> no-op, and/or 'NULL'-out", so please reference that one in the ChangeLog updates. So, eventually that'll be more than just 'libgomp/oacc-mem.c:update_dev_host', but I understand we need that one now, for Fortran optional arguments support. Any other changes can then be handled later (once the OpenACC specification changes have been completed). Please also add a new test case 'libgomp.oacc-c-c++-common/null-1.c' with a "Test 'NULL'-in -> no-op, and/or 'NULL'-out" header, executing things like 'acc_update_device (NULL, [...])' etc. for everything that calls 'update_dev_host': 'acc_update_device', 'acc_update_device_async', 'acc_update_self', 'acc_update_self_async'. These functions are also called for OpenACC 'update' directives ('libgomp/oacc-parallel.c:GOACC_update'), but I suppose it's not possible to construct an OpenACC 'update' directive conveying a 'NULL' pointer, that is, something that would result in 'hostaddrs[i] == NULL'? Likewise for Fortran, I suppose. In 'libgomp/libgomp.texi' then add a note to 'acc_update_device' (and other relevant functions) like this: @@ -2586,6 +2586,9 @@ This function updates the device copy from the previously mapped host memory. The host memory is specified with the host address @var{a} and a length of @var{len} bytes. +If @var{a} is the @code{NULL} pointer, this is a no-op. + [...] As for 'libgomp/oacc-mem.c:gomp_acc_insert_pointer', that's only called for OpenACC 'enter data' directives ('libgomp/oacc-parallel.c:GOACC_enter_exit_data'), and specifically only for 'GOMP_MAP_POINTER', 'GOMP_MAP_TO_PSET'. Is there a way to construct a test case that will result in a 'NULL' pointer there, other than via Fortran optional arguments? If not, then that hunk should be removed here, and move into/stay in the Fortran optional arguments patch that you've posted. (And that said, Julian's got a patch pending review that gets rid of 'gomp_acc_insert_pointer' and other such black magic, yay.) Grüße Thomas On 2019-07-12T12:35:05+0100, Kwok Cheung Yeung <kcy@codesourcery.com> wrote: > Fortran pass-by-reference optional arguments behave much like normal > Fortran arguments when lowered to GENERIC/GIMPLE, except they can be > null (representing a non-present argument). > > Some parts of libgomp (those dealing with updating mappings) currently > do not expect to take a null address and fail. These need to be changed > to deal with the null appropriately, by turning the operation into a > no-op (as you never need to update a non-present argument). > > libgomp/ > * oacc-mem.c (update_dev_host): Return early if the host address > is NULL. > (gomp_acc_insert_pointer): Likewise. > * testsuite/libgomp.oacc-c-c++-common/lib-43.c: Remove. > * testsuite/libgomp.oacc-c-c++-common/lib-47.c: Likewise. > --- > libgomp/oacc-mem.c | 9 ++++ > .../testsuite/libgomp.oacc-c-c++-common/lib-43.c | 51 ---------------------- > .../testsuite/libgomp.oacc-c-c++-common/lib-47.c | 49 --------------------- > 3 files changed, 9 insertions(+), 100 deletions(-) > delete mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c > delete mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c > > diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c > index 2f27100..8cc5120 100644 > --- a/libgomp/oacc-mem.c > +++ b/libgomp/oacc-mem.c > @@ -831,6 +831,12 @@ update_dev_host (int is_dev, void *h, size_t s, int async) > if (acc_dev->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM) > return; > > + /* Fortran optional arguments that are non-present result in a > + null host address here. This can safely be ignored as it is > + not possible to 'update' a non-present optional argument. */ > + if (h == NULL) > + return; > + > acc_prof_info prof_info; > acc_api_info api_info; > bool profiling_p = GOACC_PROFILING_SETUP_P (thr, &prof_info, &api_info); > @@ -901,6 +907,9 @@ gomp_acc_insert_pointer (size_t mapnum, void **hostaddrs, size_t *sizes, > struct goacc_thread *thr = goacc_thread (); > struct gomp_device_descr *acc_dev = thr->dev; > > + if (*hostaddrs == NULL) > + return; > + > if (acc_is_present (*hostaddrs, *sizes)) > { > splay_tree_key n; > diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c > deleted file mode 100644 > index 5db2912..0000000 > --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c > +++ /dev/null > @@ -1,51 +0,0 @@ > -/* Exercise acc_update_device with a NULL data address on nvidia targets. */ > - > -/* { dg-do run { target openacc_nvidia_accel_selected } } */ > - > -#include <stdio.h> > -#include <stdlib.h> > -#include <openacc.h> > - > -int > -main (int argc, char **argv) > -{ > - const int N = 256; > - int i; > - unsigned char *h; > - void *d; > - > - h = (unsigned char *) malloc (N); > - > - for (i = 0; i < N; i++) > - { > - h[i] = i; > - } > - > - d = acc_copyin (h, N); > - if (!d) > - abort (); > - > - for (i = 0; i < N; i++) > - { > - h[i] = 0xab; > - } > - > - fprintf (stderr, "CheCKpOInT\n"); > - acc_update_device (0, N); > - > - acc_copyout (h, N); > - > - for (i = 0; i < N; i++) > - { > - if (h[i] != 0xab) > - abort (); > - } > - > - free (h); > - > - return 0; > -} > - > -/* { dg-output "CheCKpOInT(\n|\r\n|\r).*" } */ > -/* { dg-output "\\\[\[^\n\r]*,256\\\] is not mapped" } */ > -/* { dg-shouldfail "" } */ > diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c > deleted file mode 100644 > index c214042..0000000 > --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c > +++ /dev/null > @@ -1,49 +0,0 @@ > -/* Exercise acc_update_self with a NULL data mapping on nvidia targets. */ > - > -/* { dg-do run { target openacc_nvidia_accel_selected } } */ > - > -#include <stdio.h> > -#include <string.h> > -#include <stdlib.h> > -#include <openacc.h> > - > -int > -main (int argc, char **argv) > -{ > - const int N = 256; > - int i; > - unsigned char *h; > - void *d; > - > - h = (unsigned char *) malloc (N); > - > - for (i = 0; i < N; i++) > - { > - h[i] = i; > - } > - > - d = acc_copyin (h, N); > - if (!d) > - abort (); > - > - memset (&h[0], 0, N); > - > - fprintf (stderr, "CheCKpOInT\n"); > - acc_update_self (0, N); > - > - for (i = 0; i < N; i++) > - { > - if (h[i] != i) > - abort (); > - } > - > - acc_delete (h, N); > - > - free (h); > - > - return 0; > -} > - > -/* { dg-output "CheCKpOInT(\n|\r\n|\r).*" } */ > -/* { dg-output "\\\[\[^\n\r]*,256\\\] is not mapped" } */ > -/* { dg-shouldfail "" } */
Hi Thomas, On 11/29/19 3:40 PM, Thomas Schwinge wrote: > "[Patch][OpenMP/OpenACC/Fortran] Fix mapping of optional (present|absent) > arguments" reminded me that still this behavioral change has not been > split out, cited below, that you described as "trivial". > > I've just filed <https://gcc.gnu.org/PR92726> "OpenACC: 'NULL'-in -> > no-op, and/or 'NULL'-out", so please reference that one in the ChangeLog > updates. > > So, eventually that'll be more than just > 'libgomp/oacc-mem.c:update_dev_host', but I understand we need that one > now, for Fortran optional arguments support. Any other changes can then > be handled later (once the OpenACC specification changes have been > completed). For the changes at hand, they are called as library for update_device(_async) and update_self(_async). And via the 'update' directives via the 'maps' alway_pointer, (force_)to and (force_)from. 'insert_pointer' is called via 'GOACC_enter_exit_data for 'enter' if the pointer can already be found; hence, I think the latter is untestable. Side note: the [update_(device|self)]_async variants are mentioned in the libgomp.texi but not really documented; they only appear at the end of "OpenACC Profiling Interface" – last block at: https://gcc.gnu.org/onlinedocs/libgomp/OpenACC-Profiling-Interface.html I have the feeling the _async variants should be properly documented in the manual; especially, the permitted values for the third argument. > Please also add a new test case 'libgomp.oacc-c-c++-common/null-1.c' with > a "Test 'NULL'-in -> no-op, and/or 'NULL'-out" header, executing things […] How about the attached patch? Tobias
diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c index 2f27100..8cc5120 100644 --- a/libgomp/oacc-mem.c +++ b/libgomp/oacc-mem.c @@ -831,6 +831,12 @@ update_dev_host (int is_dev, void *h, size_t s, int async) if (acc_dev->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM) return; + /* Fortran optional arguments that are non-present result in a + null host address here. This can safely be ignored as it is + not possible to 'update' a non-present optional argument. */ + if (h == NULL) + return; + acc_prof_info prof_info; acc_api_info api_info; bool profiling_p = GOACC_PROFILING_SETUP_P (thr, &prof_info, &api_info); @@ -901,6 +907,9 @@ gomp_acc_insert_pointer (size_t mapnum, void **hostaddrs, size_t *sizes, struct goacc_thread *thr = goacc_thread (); struct gomp_device_descr *acc_dev = thr->dev; + if (*hostaddrs == NULL) + return; + if (acc_is_present (*hostaddrs, *sizes)) { splay_tree_key n; diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c deleted file mode 100644 index 5db2912..0000000 --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-43.c +++ /dev/null @@ -1,51 +0,0 @@ -/* Exercise acc_update_device with a NULL data address on nvidia targets. */ - -/* { dg-do run { target openacc_nvidia_accel_selected } } */ - -#include <stdio.h> -#include <stdlib.h> -#include <openacc.h> - -int -main (int argc, char **argv) -{ - const int N = 256; - int i; - unsigned char *h; - void *d; - - h = (unsigned char *) malloc (N); - - for (i = 0; i < N; i++) - { - h[i] = i; - } - - d = acc_copyin (h, N); - if (!d) - abort (); - - for (i = 0; i < N; i++) - { - h[i] = 0xab; - } - - fprintf (stderr, "CheCKpOInT\n"); - acc_update_device (0, N); - - acc_copyout (h, N); - - for (i = 0; i < N; i++) - { - if (h[i] != 0xab) - abort (); - } - - free (h); - - return 0; -} - -/* { dg-output "CheCKpOInT(\n|\r\n|\r).*" } */ -/* { dg-output "\\\[\[^\n\r]*,256\\\] is not mapped" } */ -/* { dg-shouldfail "" } */ diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c deleted file mode 100644 index c214042..0000000 --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/lib-47.c +++ /dev/null @@ -1,49 +0,0 @@ -/* Exercise acc_update_self with a NULL data mapping on nvidia targets. */ - -/* { dg-do run { target openacc_nvidia_accel_selected } } */ - -#include <stdio.h> -#include <string.h> -#include <stdlib.h> -#include <openacc.h> - -int -main (int argc, char **argv) -{ - const int N = 256; - int i; - unsigned char *h; - void *d; - - h = (unsigned char *) malloc (N); - - for (i = 0; i < N; i++) - { - h[i] = i; - } - - d = acc_copyin (h, N); - if (!d) - abort (); - - memset (&h[0], 0, N); - - fprintf (stderr, "CheCKpOInT\n"); - acc_update_self (0, N); - - for (i = 0; i < N; i++) - { - if (h[i] != i) - abort (); - } - - acc_delete (h, N); - - free (h); - - return 0; -} - -/* { dg-output "CheCKpOInT(\n|\r\n|\r).*" } */ -/* { dg-output "\\\[\[^\n\r]*,256\\\] is not mapped" } */ -/* { dg-shouldfail "" } */