Hi Tobias,

Thanks for review! Here's a new version of the patch which hopefully
addresses this round of comments.

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?

Thanks,

Julian