Re: [PATCH v3 11/11] FYI/unfinished: OpenMP 5.0 "declare mapper" support for C++

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Sep 13, 2022 at 02:04:30PM -0700, Julian Brown wrote:
> This patch implements OpenMP 5.0 "declare mapper" support for C++.
> This hasn't been fully revised yet following previous review comments,
> but I am including it in this series to demonstrate the new approach to
> gimplifying map clauses after "declare mapper" instantiation.
> 
> The "gimplify_scan_omp_clauses" function is still called twice: firstly
> (before scanning the offload region's body) with SUPPRESS_GIMPLIFICATION
> set to true.  This sets up variables in the splay tree, etc. but does
> not gimplify anything.
> 
> Then, implicit "declare mappers" are instantiated after scanning the
> region's body, then "gimplify_scan_omp_clauses" is called again, and
> does the rest of its previous tasks -- builds struct sibling lists,
> and gimplifies clauses. Then gimplify_adjust_omp_clauses is called,
> and compilation continues.
> 
> Does this approach seem OK?

As I wrote in https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596444.html
I don't see a reason for this 3 passes approach and it will be a
maintainance problem.
The reason why we have 2 passes approach is that we need to populate the
splay trees based on explicit clauses before we gimplify the body, at which
point we can look up for variables seen in the body those splay trees,
either mark the explicit ones as seen or create new ones for implicit
etc.  And finally we need to adjust some explicit clauses based on that
(that is what the main loop in gimplify_adjust_omp_clauses does) and
add new clauses for implicit data sharing or mapping (that is done
through splay tree traversal through gimplify_adjust_omp_clauses_1).

We also need to gimplify expressions from the clauses somewhere, but
due to the way the gimplifier works we don't care that much when
exactly it is done, it can be done before the body is gimplified
(for most clauses we do it there in gimplify_scan_omp_clauses), it can be
done after the body is gimplified, the expressions from the clauses
will be in either case gimplified into some gimple sequence that we'll
place before the construct with its body.  The only reason to have
the gimplification done before and not after would be if the temporaries
from the gimplification are then needed in the splay trees for the
gimplification of the body.

I'd strongly prefer if the gimplification APIs for all the constructs
were just gimplify_scan_omp_clauses before processing the body and
gimplify_adjust_omp_clauses after doing so, not some extra APIs.
If there is a strong reason for 3 or more passes on say a subset of clauses,
either gimplify_scan_omp_clauses or gimplify_adjust_omp_clauses can do more
than one loop over the clauses, but those secondary loops ideally should be
enabled only when needed (e.g. see the gimplify_adjust_omp_clauses
has_inscan_reductions guarded loop at the end) and should only process
clauses they strictly have to.

Conceptually, there is no reason why e.g. the gimplification of the explicit
map clauses can't be done in gimplify_adjust_omp_clauses rather than in
gimplify_scan_omp_clauses.  What needs to happen in gimplify_scan_omp_clauses
is just what affects the gimplification of the body.  Does sorting of the
map clause affect it?  I don't think so.  Does declare mapper processing of
the explicit map clauses affect it?  I very much hope it doesn't, but I'm
afraid I don't remember all the declare mapper restrictions and discussions.
Can declare mapper e.g. try to map an unrelated variable in addition to say
parts of the structure?  If yes, then it could affect the gimplification of
the body, say
struct S { int s, t; };
extern int y;
#pragma omp declare mapper (struct S s) map (to: s.s) map (to: y)
#pragma omp target defaultmap(none:scalar) map(tofrom: x)
{
  int x = s.s + y;
}
because if we do process declare mapper before the gimplification of the
body, then y would be explicitly mapped, but if we don't, it wouldn't and
it should be rejected.  But in that case we'd be in big trouble with
implicit mappings using that same declare mapper.  Because it would then be
significant in which order we process the vars during gimplification of the
body and whether we process declare mapper right away at that point or only
at the end etc.
We have the
"List items in map clauses on the declare mapper directive may only refer to the declared
variable var and entities that could be referenced by a procedure defined at the same
location."
restriction but not sure that says the above isn't valid.
So probably it needs to be discussed in omp-lang.

If the agreement is that declare mapper for explicit map clauses needs to be
done before processing the body and declare mapper for implicit map clauses
can be deferred to the end, then yes, we need to handle declare mapper
twice, but it can be done say in a secondary loop of gimplify_scan_omp_clauses
guarded on at least one of the map clauses needs declare mapper processing
or something similar that can be quickly determined on the first loop
and can use a helper function that gimplify_scan_omp_clauses also uses.

	Jakub