Hi Tobias! Reviewing your <8be82276-81b1-817c-fcd2-51f24f5fe2d2@codesourcery.com">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 <k...@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 "" } */
signature.asc
Description: PGP signature
