Hi Jakub,
Thanks for review!
On Tue, 24 May 2022 15:03:07 +0200
Jakub Jelinek via Fortran wrote:
> On Fri, Mar 18, 2022 at 09:24:51AM -0700, Julian Brown wrote:
> > 2021-11-23 Julian Brown
> >
> > gcc/
> > * gimplify.c (is_or_contains_p,
> > omp_target_reorder_clauses): Delete functions.
> > (omp_tsort_mark): Add enum.
> > (omp_mapping_group): Add struct.
> > (debug_mapping_group, omp_get_base_pointer,
> > omp_get_attachment, omp_group_last, omp_gather_mapping_groups,
> > omp_group_base, omp_index_mapping_groups, omp_containing_struct,
> > omp_tsort_mapping_groups_1, omp_tsort_mapping_groups,
> > omp_segregate_mapping_groups, omp_reorder_mapping_groups):
> > New functions.
> > (gimplify_scan_omp_clauses): Call above functions instead of
> > omp_target_reorder_clauses, unless we've seen an error.
> > * omp-low.c (scan_sharing_clauses): Avoid strict test if we
> > haven't sorted mapping groups.
> >
> > gcc/testsuite/
> > * g++.dg/gomp/target-lambda-1.C: Adjust expected output.
> > * g++.dg/gomp/target-this-3.C: Likewise.
> > * g++.dg/gomp/target-this-4.C: Likewise.
> > +
>
> Wouldn't hurt to add a comment on the meanings of the enumerators.
Added.
> > +enum omp_tsort_mark {
> > + UNVISITED,
> > + TEMPORARY,
> > + PERMANENT
> > +};
> > +
> > +struct omp_mapping_group {
> > + tree *grp_start;
> > + tree grp_end;
> > + omp_tsort_mark mark;
> > + struct omp_mapping_group *sibling;
> > + struct omp_mapping_group *next;
> > +};
> > +
> > +__attribute__((used)) static void
>
> I'd use what is used elsewhere,
> DEBUG_FUNCTION void
> without static.
Fixed.
> > +static tree
> > +omp_get_base_pointer (tree expr)
> I must say I don't see advantages of just a single loop that
> looks through all ARRAY_REFs and all COMPONENT_REFs and then just
> checks if the expr it got in the end is a decl or INDIRECT_REF
> or MEM_REF with offset 0.
>
> > +static tree
> > +omp_containing_struct (tree expr)
> Again?
I've simplified these loops, and removed the "needs improvement"
comment.
> > @@ -9063,11 +9820,29 @@ 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)
> > -omp_target_reorder_clauses (list_p);
> > + /* Topological sorting may fail if we have duplicate nodes, which
> > + we should have detected and shown an error for already. Skip
> > + sorting in that case. */
> > + if (!seen_error ()
> > + && (code == OMP_TARGET
> > + || code == OMP_TARGET_DATA
> > + || code == OMP_TARGET_ENTER_DATA
> > + || code == OMP_TARGET_EXIT_DATA))
> > +{
> > + vec *groups;
> > + groups = omp_gather_mapping_groups (list_p);
> > + if (groups)
> > + {
> > + hash_map *grpmap;
> > + grpmap = omp_index_mapping_groups (groups);
> > + omp_mapping_group *outlist
> > + = omp_tsort_mapping_groups (groups, grpmap);
> > + outlist = omp_segregate_mapping_groups (outlist);
> > + list_p = omp_reorder_mapping_groups (groups, outlist,
> > list_p);
> > + delete grpmap;
> > + delete groups;
> > + }
> > +}
>
> I think big question is if we do want to do this map clause reordering
> before processing the omp target etc. clauses, or after (during
> gimplify_adjust_omp_clauses, when clauses from the implicit mappings
> are added too and especially with the declare mapper expansions),
> or both before and after.
The existing code constrains us a bit here, unless we want to
completely rewrite it!
We can only do sorting on clauses before gimplification, otherwise the
"structural" matching of the parsed syntax of base pointers inside other
clauses on the directive, etc. will certainly fail.
(Semi-relatedly, I asked this on the omp-lang mailing list:
"When we have mappings that represent base pointers, and other
mappings that use those base pointers, the former must be ordered to
take place before the latter -- but should we determine that relation
purely syntactically? How about if we write e.g. "p->" on one vs.
"(*p)." on the other?"
but no reply...)
So, this is fine for sorting explicit mapping clauses. When planning
the approach I've used for "declare mapper" support, I wrote this (in
an internal email):
"At the moment, gimplifying OMP workshare regions proceeds in three
phases:
1. Clauses are processed (gimplify_scan_omp_clauses), creating
records of mapped variables in a splay tree, with associated flags.
2. The body of the workshare region is processed (gimplified),
augmenting the same splay tree with information about variables
which are used implicitly (and maybe also modifying the "explicit"
mappings from the first step).
3. The clauses are modified based on the results of the second stage
(gimplify_adjust_omp_clauses). E.g. clauses are removed that refer
