[1/5,OpenACC] Allow NULL as an argument to OpenACC 2.6 directives

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

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