Re: [gomp4] Handle Fortran deviceptr clause.

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi Jim!

On Wed, 8 Jul 2015 13:00:16 -0500, James Norris <jnorris@codesourcery.com> wrote:
> This patch adds handling of the deviceptr clause when
> used within a Fortran program.

Please motivate such non-obvious code changes by a test case.  At least
to me, it's not at all obvious what's going on here...

Is that the occasion where you asked me about how to add a test case to
the libgomp testsuite, that'd consist of both a C and a Fortran part, so
you'd need to run both compilers?  (Unfortunately, I have not yet thought
about a solution for this.)  But even then, please at least post such
test cases with patch submissions.

> Committed to gomp-4_0-branch

> +	* oacc-parallel.c (GOACC_parallel GOACC_data_start): Handle Fortran
> +	deviceptr clause.

> --- a/libgomp/oacc-parallel.c
> +++ b/libgomp/oacc-parallel.c
> @@ -211,6 +211,21 @@ GOACC_parallel (int device, void (*fn) (void *),
>    thr = goacc_thread ();
>    acc_dev = thr->dev;
>  
> +  for (i = 0; i < mapnum; i++)
> +    {
> +      unsigned short kind1 = kinds[i] & 0xff;
> +      unsigned short kind2 = kinds[i+1] & 0xff;
> +
> +      if ((kind1 == GOMP_MAP_FORCE_DEVICEPTR && kind2 == GOMP_MAP_POINTER)
> +	   && (sizes[i + 1] == 0)
> +	   && (hostaddrs[i] == *(void **)hostaddrs[i + 1]))
> +	{
> +	  kinds[i+1] = kinds[i];
> +	  sizes[i+1] = sizeof (void *);
> +	  hostaddrs[i] = NULL;
> +	}
> +    }

Ugh.  That loop should be bounded by mapnum - 1 to avoid out-of-bounds
array accesses.  And, such "voodoo" code constructs do need a comment,
please.  Why does this processing need to happen at run-time, in libgomp?
Should something else be done during OMP lowering, for example?

> @@ -263,8 +278,13 @@ GOACC_parallel (int device, void (*fn) (void *),
>  
>    devaddrs = gomp_alloca (sizeof (void *) * mapnum);
>    for (i = 0; i < mapnum; i++)
> -    devaddrs[i] = (void *) (tgt->list[i]->tgt->tgt_start
> -			    + tgt->list[i]->tgt_offset);
> +    {
> +      if (tgt->list[i] != NULL)
> +	devaddrs[i] = (void *) (tgt->list[i]->tgt->tgt_start
> +				+ tgt->list[i]->tgt_offset);
> +      else
> +	devaddrs[i] = NULL;
> +    }
>  
>    acc_dev->openacc.exec_func (tgt_fn, mapnum, hostaddrs, devaddrs, sizes, kinds,
>  			      num_gangs, num_workers, vector_length, async,

> @@ -305,6 +326,21 @@ 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 < mapnum; i++)
> +    {
> +      unsigned short kind1 = kinds[i] & 0xff;
> +      unsigned short kind2 = kinds[i+1] & 0xff;
> +
> +      if ((kind1 == GOMP_MAP_FORCE_DEVICEPTR && kind2 == GOMP_MAP_POINTER)
> +	   && (sizes[i + 1] == 0)
> +	   && (hostaddrs[i] == *(void **)hostaddrs[i + 1]))
> +	{
> +	  kinds[i+1] = kinds[i];
> +	  sizes[i+1] = sizeof (void *);
> +	  hostaddrs[i] = NULL;
> +	}
> +    }

The same code being repeated here for the OpenACC data construct --
should this be moved to a common place?


GrÃÃe,
 Thomas

Attachment: signature.asc
Description: PGP signature