[Patch] Add OpenACC 2.6's no_create
Thomas Schwinge
thomas@codesourcery.com
Tue Dec 3 15:16:00 GMT 2019
Hi!
Jakub, please note question below.
On 2019-11-15T20:11:29+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> updated version. Changes:
> * Incorporate Thomas's changes
> * Add no_create clause to newly added 'acc serial'
> * Renamed (G)OMP_MAP_NO_ALLOC to (G)OMP_MAP_IF_PRESENT as proposed
> * Make no_create.c effective by adding 'has_firstprivate = true;' to
> target.c.*
Thanks.
> (* If one tries to access c or e in the no_create-3.{c,f90} run-time
> test case, plugin-nvidia rightly complains (illegal memory access),
> using the created 'b' or 'd' works as tested by the test case.
So that's specifically what you fixed above, or is that another problem?
> This
> feature seems to be also broken on the OG9 branch.)
Not surprising, given the insufficient testsuite coverage... ;'-|
I note that you've not addressed the other TODO items that I had put into
the libgomp memory mapping code (see below for reference). I still think
that this should be understood better, that the code as currently
proposed/discussed is "too complex". I have an idea how to do this
differently (easier?), but I still have to sketch that out, and not sure
when I'll get to that. I'm willing to accept that patch as-is, unless
Jakub has any further comments at this point.
Another thing: I've added just another little bit of testsuite coverage,
and another thing broke. See "TODO" in attached incremental patch.
(Please rename the files appropriately.) Please have a look.
This feels like something going wrong in gimplification, when we "Look in
outer OpenACC contexts, to see if there's a data attribute for this
variable" ('gcc/gimplify.c:omp_notice_variable'), but that's just a wild
guess. If you agree/understand that there is a problem, and add some
XFAILed 'gimple' tree-scanning test cases (maybe even just to the libgomp
test cases that I've added), I'm fine to accept that XFAILed, to be
resolved later.
Maybe even that's not specific to the 'no_create' clause, just doesn't
cause any harm (given the existing testsuite...) for other OpenACC
constructs/clauses?
The incremental Fortran test case changes have bene done in a rush; not
sure if they make much sense, or should see some further work applied to
them.
With these items considered/addressed as you feel comfortable, this is OK
for trunk. To record the review effort, please include "Reviewed-by:
Thomas Schwinge <thomas@codesourcery.com>" in the commit log, see
<https://gcc.gnu.org/wiki/Reviewed-by>.
> PS: Remaining bits of the OG9 patch, which are not included are the
> following. I think those are all attach/detach features: a test case for
> "no_create(s.y…)" (i.e. the struct component-ref;
> libgomp/testsuite/libgomp.oacc-c-c++-common/nocreate-{3,4}.c) and some
> 'do_detach = false' in libgomp/target.c. Cf. openacc-gcc-9 /…-8 branch
> patch is commit 8e74c2ec2b90819c995444370e742864a685209f of Dec 20,
> 2018. It has been posted as
> https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01418.html
The libgomp memory mapping code:
> Add OpenACC 2.6 `no_create' clause support
>
> The clause makes any device code use the local memory address for each
> of the variables specified unless the given variable is already present
> on the current device.
> --- a/include/gomp-constants.h
> +++ b/include/gomp-constants.h
> @@ -75,6 +75,8 @@ enum gomp_map_kind
> GOMP_MAP_DEVICE_RESIDENT = (GOMP_MAP_FLAG_SPECIAL_1 | 1),
> /* OpenACC link. */
> GOMP_MAP_LINK = (GOMP_MAP_FLAG_SPECIAL_1 | 2),
> + /* Use device data if present, fall back to host address otherwise. */
> + GOMP_MAP_IF_PRESENT = (GOMP_MAP_FLAG_SPECIAL_1 | 3),
> /* Do not map, copy bits for firstprivate instead. */
> GOMP_MAP_FIRSTPRIVATE = (GOMP_MAP_FLAG_SPECIAL | 0),
> /* Similarly, but store the value in the pointer rather than
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -667,6 +667,13 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
> has_firstprivate = true;
> continue;
> }
> + else if ((kind & typemask) == GOMP_MAP_IF_PRESENT)
> + {
> + tgt->list[i].key = NULL;
> + tgt->list[i].offset = 0;
> + has_firstprivate = true;
> + continue;
> + }
> cur_node.host_start = (uintptr_t) hostaddrs[i];
> if (!GOMP_MAP_POINTER_P (kind & typemask))
> cur_node.host_end = cur_node.host_start + sizes[i];
> @@ -892,6 +899,49 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
> cur_node.tgt_offset = n->tgt->tgt_start + n->tgt_offset
> + cur_node.host_start - n->host_start;
> continue;
> + case GOMP_MAP_IF_PRESENT:
> + {
> + cur_node.host_start = (uintptr_t) hostaddrs[i];
> + cur_node.host_end = cur_node.host_start + sizes[i];
> + splay_tree_key n = splay_tree_lookup (mem_map, &cur_node);
> + if (n != NULL)
> + {
> + tgt->list[i].key = n;
> + tgt->list[i].offset = cur_node.host_start - n->host_start;
> + tgt->list[i].length = n->host_end - n->host_start;
> + tgt->list[i].copy_from = false;
> + tgt->list[i].always_copy_from = false;
> + n->refcount++;
> + }
> + else
> + {
> + tgt->list[i].key = NULL;
> + tgt->list[i].offset = OFFSET_INLINED;
> + tgt->list[i].length = sizes[i];
> + tgt->list[i].copy_from = false;
> + tgt->list[i].always_copy_from = false;
> + if (i + 1 < mapnum)
> + {
> + int kind2 = get_kind (short_mapkind, kinds, i + 1);
> + switch (kind2 & typemask)
> + {
> + case GOMP_MAP_POINTER:
> + /* The data is not present but we have an attach
> + or pointer clause next. Skip over it. */
> + i++;
> + tgt->list[i].key = NULL;
> + tgt->list[i].offset = OFFSET_INLINED;
> + tgt->list[i].length = sizes[i];
> + tgt->list[i].copy_from = false;
> + tgt->list[i].always_copy_from = false;
> + break;
> + default:
> + break;
> + }
> + }
> + }
> + continue;
> + }
> default:
> break;
> }
My TODO items:
--- libgomp/target.c
+++ libgomp/target.c
@@ -671,6 +671,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
}
else if ((kind & typemask) == GOMP_MAP_IF_PRESENT)
{
+ //TODO TS is confused. Handling this here, will inhibit 'gomp_map_vars_existing' being used a bit further below.
tgt->list[i].key = NULL;
tgt->list[i].offset = 0;
has_firstprivate = true;
@@ -908,6 +910,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
splay_tree_key n = splay_tree_lookup (mem_map, &cur_node);
if (n != NULL)
{
+ //TODO TS is confused. Due to the way the handling of 'GOMP_MAP_NO_ALLOC' is done in the first loop, we're here re-doing 'gomp_map_vars_existing'?
tgt->list[i].key = n;
tgt->list[i].offset = cur_node.host_start - n->host_start;
tgt->list[i].length = n->host_end - n->host_start;
@@ -917,6 +920,7 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
}
else
{
+ //TODO This is basically 'GOMP_MAP_FIRSTPRIVATE_INT' handling?
tgt->list[i].key = NULL;
tgt->list[i].offset = OFFSET_INLINED;
tgt->list[i].length = sizes[i];
@@ -928,6 +932,11 @@ gomp_map_vars_internal (struct gomp_device_descr *devicep,
switch (kind2 & typemask)
{
case GOMP_MAP_POINTER:
+ //TODO abort();
+ //TODO This code path is exercised by 'libgomp.oacc-fortran/no_create-2.f90'.
+ //TODO TS does not yet understand why this is needed.
+ //TODO Is this somehow similar to 'GOMP_MAP_TO_PSET' handling?
+
/* The data is not present but we have an attach
or pointer clause next. Skip over it. */
i++;
Grüße
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-OpenACC-2.6-no_create-clause-support-some-.trunk.patch
Type: text/x-diff
Size: 11155 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20191203/84c0c2af/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 658 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20191203/84c0c2af/attachment.sig>
More information about the Fortran
mailing list