[Patch] Add OpenACC 2.6's no_create
Tobias Burnus
tobias@codesourcery.com
Wed Dec 18 12:41:00 GMT 2019
Hi Thomas,
@Thomas (and, possibly, Julian & Jakub): Please glance quickly the
gomp_map_vars_internal change.
libgomp/target.c's gomp_map_vars_internal: it now uses the normal code
path in the upper loop, except that one directly bails out when the
'key' has not been found (skipping the adjacent MAP_POINTER as well).
The 'case' in the second loop is only reached, if tgt[i]->key == NULL
(i.e. if not present) and one can unconditionally skip here. â This
seems to be cleaner and should avoid some confusions :-)
GOMP_MAP_POINTER, following MAP_IF_PRESENT: I am not sure about this.
The testsuite digests both mapping and skipping the map pointer. It
looks a tad cleaner to avoid mapping the pointer (if the var is not
present) â saving also few bytes and cpu cycles. On the down side, it
adds an order dependence assumption, namely assuming that the
MAP_POINTER after 'no_create'/MAP_IF_PRESENT always belongs to
no_create. â [This patch follows the original patch and skips the
map_pointer.]
Otherwise, except for added acc_is_present calls to no_create-3.c to
check that no_create does not cause mapping and applying your/Thomas's
patches, it matches my previous version, which was OK'ed. â Hence, I
intent to commit it tomorrow, unless there are further comments.
Cheers,
Tobias
On 12/17/19 8:11 PM, Tobias Burnus wrote:
> Hi Thomas,
>
> I am reasonably comfortable with the current patch (regarding your
> TODOs) â see attachment. It is the previous patch plus your changes
> plus one additional condition (see below) in target.c's first
> GOMP_MAP_IF_PRESENT handling.
>
> I intent to re-test it tomorrow and then commit it, unless some other
> issues or comments come up. â See a bunch of comments below.
>
> Cheers,
>
> Tobias
>
> On 12/3/19 4:16 PM, Thomas Schwinge wrote:
>> So that's specifically what you fixed above
> (See previous reply in this email. Now added an acc_is_present check.
> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00156.html)
>> Another thing: I've added just another little bit of testsuite
>> coverage, and another thing broke. See "TODO" in attached incremental
>> patch. [â¦]
> Files included, the other issue was XFAILed by you (and hence passed).
> A fix for that issue is:
> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01135.html â and a
> completely separate issue. (That patch is small, very localized and
> orthogonal to this patch.)
>> 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.
>
> I think one can do more, but they are fine. I am not 100% sure how to
> read the following:
>
> Â ! The no_create clause is meant for partially shared-memory
> machines. This
> Â ! test is written to work on non-shared-memory machines, though this
> is not
> Â ! necessarily a useful way to use the no_create clause in practice.
> Â !$acc parallel !no_create (var)
>
> First, why is 'no_create(var)' now commented? â For this code, it
> should really work both ways and independent whether commented boils
> down to 'copy' (currently) or 'present' (with my other patch, linked
> above).
>
>> With these items considered/addressed as you feel comfortable, this
>> is OK
>> for trunk.
>
>> 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;
>
> True â but should it? the only effect seems to be that it bumps the
> ref count. (Should it or shouldn't it?) In any case if the data is not
> present, it will fail in this section.
>
> However, I think the following is missing before 'continue' â even
> though testing did not hit it:
>
> Â Â Â Â Â /* Handle the attach/pointer clause next to it later, together with
> Â Â Â Â Â Â Â Â GOMP_MAP_IF_PRESENT as the data might be not available. */
> Â Â Â Â Â if (i + 1 < mapnum
> Â Â Â Â Â Â Â Â Â && ((typemask & get_kind (short_mapkind, kinds, i + 1))
> Â Â Â Â Â Â Â Â Â == GOMP_MAP_POINTER))
> Â Â Â Â Â Â Â ++i;
>
>> @@ -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;
> Essentially, yes â except that we know here that the variable does
> exist â in the block above, it also works, but only if the variable
> has been mapped at some point.
>> @@ -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];
> Yes â but one could also call it 'hostaddrs[i] == NULL' handling,
> which makes more sense semantically.
>> @@ -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++;
>
> Yes, as -fdump-tree-omplower shows, it is handled like a normal map,
> except that the variable itself gets a 'no_alloc'.
>
> map(no_alloc:*var.7_5 [len: 4]) map(alloc:var [pointer assign, bias:
> 0]) map(no_alloc:(*arr.8_6)
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openacc_no_create9.diff
Type: text/x-patch
Size: 38058 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20191218/ca78cd9d/attachment.bin>
More information about the Gcc-patches
mailing list