On 2021/5/17 10:26 PM, Julian Brown wrote:
OK, understood. But, I'm a bit concerned that we're ignoring some
"hidden rules" with regards to OMP pointer clause ordering/grouping that
certain code (at least the bit that creates GOMP_MAP_STRUCT node
groups, and parts of omp-low.c) relies on. I believe those rules are as
follows:
- an array slice is mapped using two or three pointers -- two for a
normal (non-reference) base pointer, and three if we have a
reference to a pointer (i.e. in C++) or an array descriptor (i.e. in
Fortran). So we can have e.g.
GOMP_MAP_TO
GOMP_MAP_ALWAYS_POINTER
GOMP_MAP_TO
GOMP_MAP_.*_POINTER
GOMP_MAP_ALWAYS_POINTER
GOMP_MAP_TO
GOMP_MAP_TO_PSET
GOMP_MAP_ALWAYS_POINTER
- for OpenACC, we extend this to allow (up to and including
gimplify.c) the GOMP_MAP_ATTACH_DETACH mapping. So we can have (for
component refs):
GOMP_MAP_TO
GOMP_MAP_ATTACH_DETACH
GOMP_MAP_TO
GOMP_MAP_TO_PSET
GOMP_MAP_ATTACH_DETACH
GOMP_MAP_TO
GOMP_MAP_.*_POINTER
GOMP_MAP_ATTACH_DETACH
For the scanning in insert_struct_comp_map (as it is at present) to
work right, these groups must stay intact. I think the current
behaviour of omp_target_reorder_clauses on the og10 branch can break
those groups apart though!
Originally this sorting was intended to enforce OpenMP 5.0 map ordering
rules, although I did add some ATTACH_DETACH ordering code in the latest
round of patching. May not be the best practice.
(The "prev_list_p" stuff in the loop in question in gimplify.c just
keeps track of the first node in these groups.)
Such a brittle way of doing this; even the variable name is not that
obvious in what it intends to do.
For OpenACC, the GOMP_MAP_ATTACH_DETACH code does*not* depend on the
previous clause when lowering in omp-low.c. But GOMP_MAP_ALWAYS_POINTER
does! And in one case ("update" directive), GOMP_MAP_ATTACH_DETACH is
rewritten to GOMP_MAP_ALWAYS_POINTER, so for that case at least, the
dependency on the preceding mapping node must stay intact.
Yes, I think there are some weird conventions here, stemming from the
front-ends.
I would think that _ALWAYS_POINTER should exist at a similar level like
_ATTACH_DETACH,
both a pointer operation, just different details in runtime behavior, though its
intended purpose for C++ references seem to skew some things here and there.
OpenACC also allows "bare" GOMP_MAP_ATTACH and GOMP_MAP_DETACH nodes
(corresponding to the "attach" and "detach" clauses). Those are handled
a bit differently to GOMP_MAP_ATTACH_DETACH in gimplify.c -- but
GOMP_MAP_ATTACH_Z_L_A_S doesn't quite behave like that either, I don't
think?
IIRC, GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION was handled that way (just a
single
line in gimplify.c) due to idiosyncrasies with the surrounding generated
maps from the C++ front-end (which ATM is the only user of this map-kind).
So yeah, inside the compiler, its not entirely the same as GOMP_MAP_ATTACH,
but it is intended to live through for the runtime to see.
Anyway: I've not entirely understood what omp_target_reorder_clauses is
doing, but I think it may need to try harder to keep the groups
mentioned above together. What do you think?
As you know, attach operations don't really need to be glued to the prior
operations, it just has to be ordered after mapping of the pointer and the
pointed.
There's already some book-keeping to move clauses together, but as you say,
it might need more.
Overall, I think this re-organizing of the struct-group creation is a good
thing,
but actually as you probably also observed, this insistence of "in-flight"
tree chain manipulation is just hard to work with and modify.
Maybe instead of directly working on clause expression chains at this point, we
should be stashing all this information into a single clause tree node,
e.g. starting from the front-end, we can set
'OMP_CLAUSE_MAP_POINTER_KIND(c) = ALWAYS/ATTACH_DETACH/FIRSTPRIVATE/etc.',
(instead of actually creating new, must-follow-in-order maps that's causing all
these conventions).
For struct-groups, during the start of gimplify_scan_omp_clauses(), we could
work
with map clause tree nodes with OMP_CLAUSE_MAP_STRUCT_LIST(c), which contains
the
entire TREE_LIST or VEC of elements. Then later, after scanning is complete,
expand the list into the current form. Ordering is only created at this stage.
Just an idea, not sure if it will help understandability in general, but it
should definitely help to simplify when we're reordering due to other rules.
Chung-Lin
