Hi Jim! On Wed, 8 Jul 2015 13:00:16 -0500, James Norris <jnor...@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
signature.asc
Description: PGP signature
