[1/4,og8] Allow NULL for update directives in OpenACC 2.6

Message ID 02e452f8-25b3-692a-c91c-74d0dd88a615@codesourcery.com
State New
Headers show
Series
  • Add support for Fortran optional arguments in OpenACC
Related show

Commit Message

Kwok Cheung Yeung Jan. 30, 2019, 10:18 p.m.
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>

---
  libgomp/ChangeLog.openacc | 5 +++++
  libgomp/oacc-mem.c        | 6 ++++++
  2 files changed, 11 insertions(+)

    acc_prof_info prof_info;
-- 
2.8.1

Comments

Thomas Schwinge Jan. 31, 2019, 5:12 p.m. | #1
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 "" } */
-- 
2.17.1

Patch

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);