Re: [gomp4] Fix Fortran deviceptr

[Cesar Philippidis <cesar@codesourcery.com>]

On 12/06/2015 06:52 AM, James Norris wrote:

> This patch fixes a some runtime issues when dealing with
> the deviceptr clause in Fortran. There were some corner
> cases that were not being dealt with correctly, and the
> patch resolves these. Also a new set of test cases has
> been added.

Which corner cases?

> diff --git a/libgomp/ChangeLog.gomp b/libgomp/ChangeLog.gomp
> index a2f1c31..791aa4c 100644
> --- a/libgomp/ChangeLog.gomp
> +++ b/libgomp/ChangeLog.gomp
> @@ -1,3 +1,10 @@
> +2015-12-06  James Norris  <jnorris@codesourcery.com>
> +
> +	* oacc-parallel.c (GOACC_parallel_keyed, GOACC_data_start):
> +	Handle Fortran deviceptr clause combination.
> +	* testsuite/libgomp.oacc-fortran/deviceptr-1.f90: New test.
> +	* testsuite/libgomp.oacc-fortran/declare-1.f90: Remove erroneous test.
> +
>  2015-12-05  Chung-Lin Tang  <cltang@codesourcery.com>
>  
>  	* oacc-plugin.h (GOMP_PLUGIN_async_unmap_vars): Add int parameter.
> diff --git a/libgomp/oacc-parallel.c b/libgomp/oacc-parallel.c
> index a4b2c01..a606152 100644
> --- a/libgomp/oacc-parallel.c
> +++ b/libgomp/oacc-parallel.c
> @@ -99,18 +99,37 @@ GOACC_parallel_keyed (int device, void (*fn) (void *),
>    thr = goacc_thread ();
>    acc_dev = thr->dev;
>  
> -  for (i = 0; i < (signed)(mapnum - 1); i++)
> +  for (i = 0; i < mapnum; i++)
>      {
>        unsigned short kind1 = kinds[i] & 0xff;
> -      unsigned short kind2 = kinds[i+1] & 0xff;
>  
>        /* Handle Fortran deviceptr clause.  */
> -      if ((kind1 == GOMP_MAP_FORCE_DEVICEPTR && kind2 == GOMP_MAP_POINTER)
> -	   && (sizes[i + 1] == 0)
> -	   && (hostaddrs[i] == *(void **)hostaddrs[i + 1]))
> +      if (kind1 == GOMP_MAP_FORCE_DEVICEPTR)
>  	{
> -	  kinds[i+1] = kinds[i];
> -	  sizes[i+1] = sizeof (void *);
> +	  unsigned short kind2;
> +
> +	  if (i < (signed)mapnum - 1)
> +	    kind2 = kinds[i + 1] & 0xff;
> +	  else
> +	    kind2 = 0xffff;
> +
> +	  if (sizes[i] == sizeof (void *))
> +	    continue;
> +
> +	  /* At this point, we're dealing with a Fortran deviceptr.
> +	     If the next element is not what we're expecting, then
> +	     this is an instance of where the deviceptr variable was
> +	     not used within the region and the pointer was removed
> +	     by the gimplifier.  */
> +	  if (kind2 == GOMP_MAP_POINTER
> +	      && sizes[i + 1] == 0
> +	      && hostaddrs[i] == *(void **)hostaddrs[i + 1])
> +	    {
> +	      kinds[i+1] = kinds[i];
> +	      sizes[i+1] = sizeof (void *);
> +	    }
> +
> +	  /* Invalidate the entry.  */
>  	  hostaddrs[i] = NULL;
>  	}
>      }
> @@ -254,18 +273,38 @@ GOACC_data_start (int device, size_t mapnum,
>    struct goacc_thread *thr = goacc_thread ();
>    struct gomp_device_descr *acc_dev = thr->dev;
>  
> -  for (i = 0; i < (signed)(mapnum - 1); i++)
> +  for (i = 0; i < mapnum; i++)
>      {
>        unsigned short kind1 = kinds[i] & 0xff;
> -      unsigned short kind2 = kinds[i+1] & 0xff;
>  
>        /* Handle Fortran deviceptr clause.  */
> -      if ((kind1 == GOMP_MAP_FORCE_DEVICEPTR && kind2 == GOMP_MAP_POINTER)
> -	   && (sizes[i + 1] == 0)
> -	   && (hostaddrs[i] == *(void **)hostaddrs[i + 1]))
> +      if (kind1 == GOMP_MAP_FORCE_DEVICEPTR)
>  	{
> -	  kinds[i+1] = kinds[i];
> -	  sizes[i+1] = sizeof (void *);
> +	  unsigned short kind2;
> +
> +	  if (i < (signed)mapnum - 1)
> +	    kind2 = kinds[i + 1] & 0xff;
> +	  else
> +	    kind2 = 0xffff;
> +
> +	  /* If the size is right, skip it.  */
> +	  if (sizes[i] == sizeof (void *))
> +	    continue;
> +
> +	  /* At this point, we're dealing with a Fortran deviceptr.
> +	     If the next element is not what we're expecting, then
> +	     this is an instance of where the deviceptr variable was
> +	     not used within the region and the pointer was removed
> +	     by the gimplifier.  */
> +	  if (kind2 == GOMP_MAP_POINTER
> +	      && sizes[i + 1] == 0
> +	      && hostaddrs[i] == *(void **)hostaddrs[i + 1])
> +	    {
> +	      kinds[i+1] = kinds[i];
> +	      sizes[i+1] = sizeof (void *);
> +	    }
> +
> +	  /* Invalidate the entry.  */
>  	  hostaddrs[i] = NULL;
>  	}
>      }

Two observations:

 1. Why is deviceptr so special that gomp_map_vars can't handle it
    directly?

 2. It appears that deviceptr code in GOACC_parallel_keyed is mostly
    identical to GOACC_data_start. Can you put that duplicate code into
    a function? That would be easier to maintain in the long run.

> diff --git a/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90 b/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90
> index 430cd24..e781878 100644
> --- a/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/declare-1.f90
> @@ -1,6 +1,4 @@
>  ! { dg-do run  { target openacc_nvidia_accel_selected } }
> -! libgomp: cuStreamSynchronize error: an illegal memory access was encountered
> -! { dg-xfail-run-if "TODO" { *-*-* } }

Maybe add a comment to describe what the test is doing like you did ...

> diff --git a/libgomp/testsuite/libgomp.oacc-fortran/deviceptr-1.f90 b/libgomp/testsuite/libgomp.oacc-fortran/deviceptr-1.f90
> new file mode 100644
> index 0000000..879cbf1
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/deviceptr-1.f90
> @@ -0,0 +1,197 @@
> +! { dg-do run }
> +
> +!! Test the deviceptr clause with various directives
> +!! and in combination with other directives where
> +!! the deviceptr variable is implied.

... here. And minor nit, but the other fortran tests only use a single
exclamation point for comments. This test should also use a single one
for consistency.

Cesar