Hi Jakub,
On Tue, 4 Dec 2018 15:17:08 +0100
Jakub Jelinek <ja...@redhat.com> wrote:
> On Mon, Sep 03, 2018 at 08:46:54PM -0400, Julian Brown wrote:
> > 2018-09-03 Cesar Philippidis <ce...@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
