Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc

[Tobias Burnus <tobias@codesourcery.com>]

Hi Julian,

On 20.12.23 22:29, Julian Brown wrote:
> Thanks for review! Here's a new version of the patch which hopefully
> addresses this round of comments.

Thanks for the patch. LGTM now.

Tobias

> On Tue, 19 Dec 2023 16:41:54 +0100
> Tobias Burnus <tobias@codesourcery.com> wrote:
>
>> On 16.12.23 14:25, Julian Brown wrote:
>>> --- a/gcc/gimplify.cc
>>> +++ b/gcc/gimplify.cc
>>> @@ -10107,6 +10114,20 @@ omp_segregate_mapping_groups
>>> (omp_mapping_group *inlist) ard_tail = &w->next;
>>>          break;
>>>
>>> +     case GOMP_MAP_PRESENT_ALLOC:
>>> +       *pa_tail = w;
>>> +       w->next = NULL;
>>> +       pa_tail = &w->next;
>>> +       break;
>>> +
>>> +     case GOMP_MAP_PRESENT_FROM:
>>> +     case GOMP_MAP_PRESENT_TO:
>>> +     case GOMP_MAP_PRESENT_TOFROM:
>>> +       *ptf_tail = w;
>>> +       w->next = NULL;
>>> +       ptf_tail = &w->next;
>>> +       break;
>>> +
>> First, I note that GOMP_MAP_PRESENT_ALLOC and
>> GOMP_MAP_PRESENT_{FROM,TO,TOFROM} are semantically identical: If the
>> variable is not present, error termination will happen - otherwise, if
>> present, no data movement will happen. Hence, they will be changed to
>> GOMP_MAP_FORCE_PRESENT in gimplify_adjust_omp_clauses.
>>
>> That's also the reason that the old code handled all of them
>> identical.
>>
>> However, besides a plain 'present', there is also 'always' +
>> 'present'. Those are different as after a normal 'present' check
>> (abort if not present), the data copying will happen:
>> GOMP_MAP_ALWAYS_PRESENT_TO, GOMP_MAP_ALWAYS_PRESENT_FROM,
>> GOMP_MAP_ALWAYS_PRESENT_TOFROM.
>>
>> (Note that: always + present + alloc = GOMP_MAP_PRESENT_ALLOC (w/o
>> 'always') as already done in the FE.)
>>
>> Thus, all 'case' from your patch should go to a single group (possibly
>> adding a comment about it). The question is what to do with the
>> 'present,always' case. I think leaving them under 'default:' is fine,
>> but I might have missed something.
> I've made this change (i.e.: grouping all "GOMP_MAP_PRESENT_*" nodes
> together), and in fact that restores the dump output for the
> gfortran.dg/gomp/map-12.f90 that needed to be adjusted for the previous
> version of the patch (so that hunk has now disappeared).
>
>>>        default:
>>>          *tf_tail = w;
>>>          w->next = NULL;
>>> @@ -10118,8 +10139,10 @@ omp_segregate_mapping_groups
>>> (omp_mapping_group *inlist)
>> * * *
>>> @@ -11922,119 +11945,30 @@ gimplify_scan_omp_clauses (tree *list_p,
>>> gimple_seq *pre_p, break;
>>>          }
>>>
>>> -  if (code == OMP_TARGET
>>> -      || code == OMP_TARGET_DATA
>>> -      || code == OMP_TARGET_ENTER_DATA
>>> -      || code == OMP_TARGET_EXIT_DATA)
>>> -    {
>>> -      vec<omp_mapping_group> *groups;
>>> -      groups = omp_gather_mapping_groups (list_p);
>>> -      if (groups)
>>> -     {
>>> -       hash_map<tree_operand_hash_no_se, omp_mapping_group *>
>>> *grpmap;
>>> -       grpmap = omp_index_mapping_groups (groups);
>>> +  vec<omp_mapping_group> *groups = omp_gather_mapping_groups
>>> (list_p);
>>> +  hash_map<tree_operand_hash_no_se, omp_mapping_group *> *grpmap =
>>> NULL;
>>> +  unsigned grpnum = 0;
>>> +  tree *grp_start_p = NULL, grp_end = NULL_TREE;
>> ...
>>
>>> -  else if (region_type & ORT_ACC)
>>> -    {
>> I wonder whether you should not better call
>> 'omp_gather_mapping_groups' only for the 'code == OMP_TARGET...' and
>> for ORT_ACC (or some subset of OACC *), given that this function is
>> also called bygimplify_omp_parallel, gimplify_omp_task,
>> gimplify_omp_for, ...
>>
>> This avoids some memory allocation and list_p walking, i.e. it is not
>> too bad - but also not really needed for task, parallel, for, ...
> I've made that change -- OpenACC uses OMP_CLAUSE_MAP in quite a wide
> range of directives, but the new version of the patch lists them
> individually anyway, rather than using a catch-all for ORT_ACC regions.
> That seems OK, I think.
>
>>> @@ -14008,26 +13926,73 @@ gimplify_adjust_omp_clauses (gimple_seq
>>> *pre_p, gimple_seq body, tree *list_p, default:
>>>              break;
>>>            }
>>> -       if (code == OMP_TARGET_EXIT_DATA
>>> -           && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_POINTER)
>>> +       switch (code)
>>>            {
>>> +         case OMP_TARGET:
>>> +           break;
>>> +         case OACC_DATA:
>>> +           if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
>>> +             break;
>>> +           goto check_firstprivate;
>>> +         case OACC_ENTER_DATA:
>>> +         case OACC_EXIT_DATA:
>>> +         case OMP_TARGET_DATA:
>>> +         case OMP_TARGET_ENTER_DATA:
>>> +         case OMP_TARGET_EXIT_DATA:
>>> +         case OACC_HOST_DATA:
>>> +         check_firstprivate:
>>> +           if (OMP_CLAUSE_MAP_KIND (c) ==
>>> GOMP_MAP_FIRSTPRIVATE_POINTER
>> I think it looks nicer if the OACC_HOST is before OMP_* such that all
>> OACC_* are together. (In the old code, oacc_enter/exit was treated
>> differently than OMP_* and OACC_HOST_DATA; your order is a leftover
>> from that code movement/change.)
> I've fixed this bit -- which actually doesn't need the goto any more
> either, so that's now a fallthrough instead.
>
>>> +         flags = GOVD_MAP | GOVD_EXPLICIT;
>>> +         if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_TO
>>> +             || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_TOFROM)
>>> +           flags |= GOVD_MAP_ALWAYS_TO;
>> I know that the code has only been moved, but I wonder whether that
>> should also include GOMP_MAP_ALWAYS_PRESENT_{TO,TOFROM} as condition.
> I've added it (caveat: without any tests).
>
> Re-tested with offloading to NVPTX. OK now?
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955