| Message ID | 02e452f8-25b3-692a-c91c-74d0dd88a615@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Fortran optional arguments in OpenACC | expand |
Hi Kwok! On Wed, 30 Jan 2019 22:18:43 +0000, Kwok Cheung Yeung <kcy@codesourcery.com> wrote: > A non-present passed-by-reference Fortran optional argument is > represented by a null pointer. When passed to an update directive, it > should be ignored as variable mappings are not created for null > pointers. This should be safe as it is not possible to change a > non-present argument into a present one (or vice-versa) in Fortran. > > libgomp/ > * oacc-mem.c (update_dev_host): Return early if the host address > is NULL. > > Reviewed-by: Julian Brown <julian@codesourcery.com> > Reviewed-by: Thomas Schwinge <thomas@codesourcery.com> As I'd mentioned a few weeks ago internally, this change: > --- a/libgomp/oacc-mem.c > +++ b/libgomp/oacc-mem.c > @@ -819,6 +819,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; ... doesn't just affect the Fortran "update" directive but also the C/C++ one, and the "acc_update_device", "acc_update_self" runtime library functions, and thus invalidates the C/C++ "libgomp.oacc-c-c++-common/lib-43.c", and "libgomp.oacc-c-c++-common/lib-47.c" test cases, so these will have to be adjusted (that is, removed). I pushed the attached to openacc-gcc-8-branch in commit 745d3a19c63ec9c1de3f44e0dd5ee3ff126e2828 "Remove libgomp.oacc-c-c++-common/lib-43.c, libgomp.oacc-c-c++-common/lib-47.c". For avoidance of doubt: I'm not objecting to your code change. On contrary, I do have an issue open to verify whether a host-NULL should actually be accepted in even more places, as a no-op. But we'll have to review/adjust/test all the code, not just "update_dev_host". (That's for later, of course.) A (very) quick check with the PGI compiler shows that they're also handling such NULL pointers as no-ops. Grüße Thomas From 745d3a19c63ec9c1de3f44e0dd5ee3ff126e2828 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <thomas@codesourcery.com> Date: Thu, 31 Jan 2019 18:08:09 +0100 Subject: [PATCH] Remove libgomp.oacc-c-c++-common/lib-43.c, libgomp.oacc-c-c++-common/lib-47.c They're invalid after commit 550afd2b2b47091f43780f0fbf5ead87f73d9b1e "[og8] Allow NULL for update directives in OpenACC 2.6". libgomp/ * testsuite/libgomp.oacc-c-c++-common/lib-43.c: Remove. * testsuite/libgomp.oacc-c-c++-common/lib-47.c: Likewise. --- libgomp/ChangeLog.openacc | 3 ++ .../libgomp.oacc-c-c++-common/lib-43.c | 51 ------------------- .../libgomp.oacc-c-c++-common/lib-47.c | 49 ------------------ 3 files changed, 3 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/ChangeLog.openacc b/libgomp/ChangeLog.openacc index d558f0df97b..edb7d3f76eb 100644 --- a/libgomp/ChangeLog.openacc +++ b/libgomp/ChangeLog.openacc @@ -1,5 +1,8 @@ 2019-01-31 Thomas Schwinge <thomas@codesourcery.com> + * testsuite/libgomp.oacc-c-c++-common/lib-43.c: Remove. + * testsuite/libgomp.oacc-c-c++-common/lib-47.c: Likewise. + * testsuite/libgomp.oacc-c-c++-common/avoid-offloading-1.c: Update. * testsuite/libgomp.oacc-c-c++-common/avoid-offloading-2.c: 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 5db29124e9e..00000000000 --- 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 c2140429cb1..00000000000 --- 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 "" } */
diff --git a/libgomp/ChangeLog.openacc b/libgomp/ChangeLog.openacc index a9a30d2..69bd1ee 100644 --- a/libgomp/ChangeLog.openacc +++ b/libgomp/ChangeLog.openacc @@ -1,3 +1,8 @@ +2019-01-30 Kwok Cheung Yeung <kcy@codesourcery.com> + + * oacc-mem.c (update_dev_host): Return early if the host address + is NULL. + 2019-01-30 Andrew Jenner <andrew@codesourcery.com> * testsuite/libgomp.fortan/fortran.exp (lang_link_flags): Add diff --git a/libgomp/oacc-mem.c b/libgomp/oacc-mem.c index 9b70820..74d7ce9 100644 --- a/libgomp/oacc-mem.c +++ b/libgomp/oacc-mem.c @@ -819,6 +819,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; + gomp_mutex_lock (&acc_dev->lock);