Re: [gomp4] Handle Fortran deviceptr clause.

[James Norris <jnorris at codesourcery dot com>]

Hi Thomas,

On 07/09/2015 03:29 AM, Thomas Schwinge wrote:
> 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...

Attached are two files which allow the testing of the fix.
I wasn't able to figure out how to get the testsuite to
compile a c source file and a fortran source file. So the
following should suffice until I figure something out.


/opt/codesourcery/trunk/bin/x86_64-none-linux-gnu-gcc -fopenacc -Wall -c 
dp-1.c
/opt/codesourcery/trunk/bin/x86_64-none-linux-gnu-gfortran -fopenacc 
-Wall -c dp-2.f90
/opt/codesourcery/trunk/bin/x86_64-none-linux-gnu-gcc -fopenacc -Wall -o 
dp dp-1.o dp-2.o


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

The 'Ugh' is fixed, comment added, and committed to gomp-4_0-branch.

> Why does this processing need to happen at run-time, in libgomp?
> Should something else be done during OMP lowering, for example?

The code in question is for the handling of the deviceptr clause
when presented via Fortran. As there is special code to handle
PSETs, which are Fortran specific, in the run-time, I felt that
there shouldn't be an issue with handling the deviceptr clause
there as well.

Thank you, thank you,
Jim

diff --git a/libgomp/oacc-parallel.c b/libgomp/oacc-parallel.c
index eeb08c4..91a5e7d 100644
--- a/libgomp/oacc-parallel.c
+++ b/libgomp/oacc-parallel.c
@@ -187,7 +187,7 @@ GOACC_parallel (int device, void (*fn) (void *),
   struct gomp_device_descr *acc_dev;
   struct target_mem_desc *tgt;
   void **devaddrs;
-  unsigned int i;
+  int i;
   struct splay_tree_key_s k;
   splay_tree_key tgt_fn_key;
   void (*tgt_fn);
@@ -211,11 +211,12 @@ GOACC_parallel (int device, void (*fn) (void *),
   thr = goacc_thread ();
   acc_dev = thr->dev;
 
-  for (i = 0; i < mapnum; i++)
+  for (i = 0; i < (signed)(mapnum - 1); 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]))
@@ -326,11 +327,12 @@ 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++)
+  for (i = 0; i < (signed)(mapnum - 1); 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]))

Attachment: dp-1.c
Description: Text document

Attachment: dp-2.f90
Description: Text document