Hi Tobias!

On Wed, 6 Dec 2023 12:36:34 +0100
Tobias Burnus <tobias@codesourcery.com> wrote:

> Hi Julian,
> 
> LGTM, except for:
> 
> * The 'target exit data' handling - comments below - looks a bit
> fishy/inconsistent.
> 
> I intent to have a closer look with more code context, but maybe you
> should have a look at it as well.
> 
> BTW: Fortran deep-mapping is not yet on mainline. Are you aware of
> changes or in particular testcases on OG13 related to your patch
> series that should be included when upstreaming that auto-mapping of
> allocatable components patch?

I thought I'd adjusted some tests to use "pointer" instead of
"allocatable" at some point for mainline submission, but now I can't
find where I'm thinking of.  So possibly.  (I'll keep an eye out...)

> > +      if (OMP_CLAUSE_MAP_KIND (node) == GOMP_MAP_DELETE
> > +       || OMP_CLAUSE_MAP_KIND (node) == GOMP_MAP_RELEASE
> > +       || op == EXEC_OMP_TARGET_EXIT_DATA)
> >       {
> > [...]
> > +       gomp_map_kind map_kind
> > +         = (op == EXEC_OMP_TARGET_EXIT_DATA) ? GOMP_MAP_RELEASE
> > +                                             : OMP_CLAUSE_MAP_KIND
> > (node);
> > +       OMP_CLAUSE_SET_MAP_KIND (node2, map_kind);
> > +       OMP_CLAUSE_RELEASE_DESCRIPTOR (node2) = 1;  
> 
> For '!$omp target exit data map(delete: array)' this looks wrong as it
> replaces 'delete' by 'release' for the descriptor - while for
>   '!$omp target (data) map(delete: array)'
> it remains 'delete'.
> 
> Thus, I wonder whether that shouldn't be instead
>    OMP_CLAUSE_MAP_KIND (node) == GOMP_MAP_DELETE
>    ? GOMP_MAP_DELETE : GOMP_MAP_RELEASE;

I've fixed that as you suggest.  Actually I've made OpenACC use the new
node layout as well, since (a) it works and (b) it was weirdly
inconsistent before.  That is, exit data directives will no longer use
e.g.:

  GOMP_MAP_FROM
  GOMP_MAP_TO_PSET
  GOMP_MAP_ATTACH_DETACH

but instead,

  GOMP_MAP_FROM
  GOMP_MAP_RELEASE (with OMP_CLAUSE_RELEASE_DESCRIPTOR set)
  GOMP_MAP_ATTACH_DETACH

actually the current state is that GOMP_MAP_TO_PSET will be used for
the descriptor on an "exit data" directive if you refer to the whole
array, but GOMP_MAP_RELEASE (etc.) will be used if you refer to an array
section (without the flag newly added in this patch, of course). I
don't think there's any reason to maintain that inconsistency.

> We later have the following; just reading the patch, I wonder whether
> GOMP_TO_PSET is correct for a generic 'target exit data' here or not.
>  It seems at a glance as if an "|| op == 'target exit data'" is
> missing here:
> 
> > -                       if (openacc)
> > -                         OMP_CLAUSE_SET_MAP_KIND (desc_node,
> > +                       if (openacc
> > +                           || (map_kind != GOMP_MAP_RELEASE
> > +                               && map_kind != GOMP_MAP_DELETE))
> > +                         OMP_CLAUSE_SET_MAP_KIND (node2,
> >                                                    GOMP_MAP_TO_PSET);
> >                         else
> > -                         OMP_CLAUSE_SET_MAP_KIND (desc_node,
> > map_kind);
> > -                       OMP_CLAUSE_DECL (desc_node) = inner;
> > -                       OMP_CLAUSE_SIZE (desc_node) =
> > TYPE_SIZE_UNIT (type);
> > -                       if (openacc)
> > -                         node2 = desc_node;
> > -                       else
> > +                         OMP_CLAUSE_SET_MAP_KIND (node2,
> > map_kind);  
> 
> (Here, without setting OMP_CLAUSE_RELEASE_DESCRIPTOR.)
> 
> And for completeness, we later have:
> 
> > +omp_map_clause_descriptor_p (tree c)
> > +{
> > +  if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP)
> > +    return false;
> > +
> > +  if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_TO_PSET)
> > +    return true;
> > +
> > +  if ((OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_RELEASE
> > +       || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DELETE)
> > +      && OMP_CLAUSE_RELEASE_DESCRIPTOR (c))
> > +    return true;  

In that section, the "exit data" directive case is handled a few lines
higher up the function, with a condition that (now) reads:

  else if (op == EXEC_OMP_TARGET_EXIT_DATA
           || op == EXEC_OACC_EXIT_DATA)
    map_kind = GOMP_MAP_RELEASE;

so I think we're OK -- unless I missed something?  (Removing the
OpenACC special case and inverting a conditional simplifies the logic
here a bit.)

> > +                               /* Save array descriptor for use
> > +                                  in
> > gfc_omp_deep_mapping{,_p,_cnt}; force
> > +                                  evaluate to ensure that it is
> > +                                  not gimplified + is a decl.  */  
> This is part of my Fortran allocatable-components deep-mapping patch
> that is currently only on OG9 (?) to OG13, badly needs to be
> upstreamed but required that Jakub had a look at it - well, I still
> would like that he has a look at the omp-low.cc parts and it needs to
> be re-diffed.
> 
> Hence, while I wouldn't mind to keep it to avoid more diffing work, I
> think it will be cleaner not to keep it here.

Agreed -- that looks like a bad bit of merge resolution on my part, in
fact.  I've removed it from this patch.

I've re-tested this version.  Does it look better now?

Thanks,

Julian