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