Re: [PATCH v2 01/11] OpenMP 5.0: Clause ordering for OpenMP 5.0 (topological sorting by base pointer)

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Jun 08, 2022 at 04:00:39PM +0100, Julian Brown wrote:
> > 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
>     to variables that aren't actually used in the region, or new
>     clauses created for implicitly-referenced variables without mapping
>     clauses on the construct.
> 
> The problem with this with regards to mappers is that the "expanded"
> mappers should undergo some of the processing we currently perform
> during phase 1 (struct sibling list handling, and so on), but we don't
> know which variables are implicitly referenced until phase 2.
> 
> [description of a plan that didn't work removed]
> 
> So the new plan is to do:
> 
> phase 1  (scan original clauses)
> phase 2  (scan workshare body)
> phase 1  (use variables from "2" to instantiate mappers, and process
>           new clauses only. Prepend new list to original clauses)
> phase 3  (as before)
> 
> I was concerned that this would upset the sorting code -- but I think
> actually, as long as implicitly-created clauses are inserted at the
> front of the clause list, there can't be a case where a pointer base is
> mapped after a use of that base. If that assumption turns out to be
> wrong, then things might get a little more complicated."
> 
> ...and so far, the plan seems to be working out. The assumption, to
> state it in other words, is that an implicitly-added map clause *cannot*
> have a dependency on an explicit map clause, in terms of relying on a
> base pointer in that explicit clause, by construction.

I don't think there is any need to add extra phases, but we can move
some code from gimplify_scan_omp_clauses to gimplify_adjust_omp_clauses.
What must be done in gimplify_scan_omp_clauses is stuff that will or
could affect the gimplification of the region's body, in that phase 2
we want to know say that some variable was privatized explicitly or
explicitly mapped or none of that, so we can based on that decide if we
should note implicit data sharing or implicit mapping etc.
But e.g. the sorting of the OMP_CLAUSE_MAP clauses is something that can
IMHO be deferred until we have all those clauses, probably it is done
in gimplify_scan_omp_clauses right now was just that the sorting at least
initially was only needed for struct mapping (map (tofrom: a.b, a.c, a.d.e, a.d.f))
and that could appear only explicitly, not implicitly, implicit mapping
would only map the whole var.
But declare mapper changes this substantially, declare mapper can add
similar mappings even from the implicit maps.
So, I think we should keep in phase 1 for OMP_CLAUSE_MAP only the stuff that
perhaps gimplifies some expressions used in those and puts records about
them into splay trees and sorting and ideally some kind of merging of
adjacent mappings can be done only when we have even the implicit
mappings all collected (so that would be after
  splay_tree_foreach (ctx->variables, gimplify_adjust_omp_clauses_1, &data);
finishes).

	Jakub