Re: [PATCH, OpenACC] Support Fortran derived type members in "acc update" directives

[Julian Brown <julian@codesourcery.com>]

Hi Jakub,

On Tue, 4 Dec 2018 15:17:08 +0100
Jakub Jelinek <jakub@redhat.com> wrote:

> On Mon, Sep 03, 2018 at 08:46:54PM -0400, Julian Brown wrote:
> > 2018-09-03  Cesar Philippidis  <cesar@codesourcery.com>
> > 
> >         gcc/fortran/
> >         * openmp.c (gfc_match_omp_variable_list): New allow_derived
> >         argument. (gfc_match_omp_map_clause): Update call to
> >         gfc_match_omp_variable_list. (gfc_match_omp_clauses): Update
> >         calls to gfc_match_omp_map_clause. (gfc_match_oacc_update):
> >         Update call to gfc_match_omp_clauses. (resolve_omp_clauses):
> >         Permit derived type variables in ACC UPDATE clauses.
> >         * trans-openmp.c (gfc_trans_omp_clauses_1): Lower derived
> > type members.
> > 
> >         gcc/
> >         * gimplify.c (gimplify_scan_omp_clauses): Update handling
> > of ACC UPDATE variables.
> > 
> >         gcc/testsuite/
> >         * gfortran.dg/goacc/derived-types.f90: New test.
> > 
> >         libgomp/
> >         * testsuite/libgomp.oacc-fortran/update-2.f90: New test.
> >         * testsuite/libgomp.oacc-fortran/derived-type-1.f90: New
> > test.  
> 
> Note, already OpenMP 4.5 allows the %s in map/to/from clauses, I just
> didn't get to that yet.
> And OpenMP 5.0 allows arbitrary expressions there.
> 
> > @@ -4336,9 +4342,12 @@ resolve_omp_clauses (gfc_code *code,
> > gfc_omp_clauses *omp_clauses, || n->expr->ref == NULL
> >  			|| n->expr->ref->next
> >  			|| n->expr->ref->type != REF_ARRAY)
> > -		      gfc_error ("%qs in %s clause at %L is not a
> > proper "
> > -				 "array section", n->sym->name,
> > name,
> > -				 &n->where);
> > +		      {
> > +			if (n->sym->ts.type != BT_DERIVED)
> > +			  gfc_error ("%qs in %s clause at %L is
> > not a proper "
> > +				     "array section",
> > n->sym->name, name,
> > +				     &n->where);
> > +		      }
> >  		    else if (n->expr->ref->u.ar.codimen)
> >  		      gfc_error ("Coarrays not supported in %s
> > clause at %L", name, &n->where);  
> 
> I'm worried about this change a little bit.  It isn't guarded for
> OpenACC only and I wonder if you actually resolve properly the
> derived expressions (look inside of those).
> 
> > diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
> > index f038f4c..95b15e5 100644
> > --- a/gcc/fortran/trans-openmp.c
> > +++ b/gcc/fortran/trans-openmp.c
> > @@ -2108,7 +2108,68 @@ gfc_trans_omp_clauses (stmtblock_t *block,
> > gfc_omp_clauses *clauses, tree decl = gfc_get_symbol_decl (n->sym);
> >  	      if (DECL_P (decl))
> >  		TREE_ADDRESSABLE (decl) = 1;
> > -	      if (n->expr == NULL || n->expr->ref->u.ar.type ==
> > AR_FULL)
> > +	      /* Handle derived-typed members for OpenACC Update.
> > */
> > +	      if (n->sym->ts.type == BT_DERIVED
> > +		  && n->expr != NULL && n->expr->ref != NULL
> > +		  && (n->expr->ref->next == NULL
> > +		      || (n->expr->ref->next != NULL
> > +			  && n->expr->ref->next->type == REF_ARRAY
> > +			  && n->expr->ref->next->u.ar.type ==
> > AR_FULL))
> > +		  && (n->expr->ref->type == REF_ARRAY
> > +		      && n->expr->ref->u.ar.type != AR_SECTION))  
> 
> Like here you have all kinds of conditions, but has resolving made
> sure all the needed diagnostics is emitted?
> Perhaps at least for now this also should be guarded on OpenACC only,
> once OpenMP allows %s in map/to/from, part of this will be usable for
> it, but e.g.
> 
> > +		  if (context != type)
> > +		    {
> > +		      tree f2 = c->norestrict_decl;
> > +		      if (!f2 || DECL_FIELD_CONTEXT (f2) != type)
> > +			for (f2 = TYPE_FIELDS (TREE_TYPE (decl));
> > f2;
> > +			     f2 = DECL_CHAIN (f2))
> > +			  if (TREE_CODE (f2) == FIELD_DECL
> > +			      && DECL_NAME (f2) == DECL_NAME
> > (field))
> > +			    break;
> > +		      gcc_assert (f2);
> > +		      c->norestrict_decl = f2;
> > +		      field = f2;
> > +		    }  
> 
> the above stuff looks way too OpenACC specific.

Thanks for the review! As it happened though, I had to rewrite a lot of
the code in this patch for the attach/detach patch, and I had meant to
withdraw this one. Many apologies about the wasted time! I mentioned
the superseding in the first submission of the attach/detach patch:

  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00826.html

but omitted to follow up with a link back from this patch to that one.
A revised version of the attach/detach patch is here:

  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02556.html

OpenACC 2.6 allows derived type member accesses (or structs, etc.) on
more types of directive than just "update", so this patch wasn't
sufficient -- for the new code replacing the bits in this patch (i.e.
the bits under gcc/fortran), I tried to integrate with the existing
code a little better, hopefully without disturbing the OpenMP side too
much.

I transferred the new tests from this patch over to the attach/detach
patch also, where of course they pass.

Cheers,

Julian