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 <jnor...@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 <clt...@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
