| Message ID | 04b90981e94acbabde004ee0992d404931e4fabf.1620721888.git.julian@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | OpenACC/OpenMP: Rework struct component handling | expand |
On 2021/5/11 4:57 PM, Julian Brown wrote: > This work-in-progress patch tries to get > GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION to behave more like > GOMP_MAP_ATTACH_DETACH -- in that the mapping is made to form groups > to be processed by build_struct_group/build_struct_comp_map. I think > that's important to integrate with how groups of mappings for array > sections are handled in other cases. > > This patch isn't sufficient by itself to fix a couple of broken test cases > at present (libgomp.c++/target-lambda-1.C, libgomp.c++/target-this-4.C), > though. No, GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION is supposed to be just a slightly different behavior version of GOMP_MAP_ATTACH; it tolerates an unmapped pointer-target and assigns NULL on the device, instead of just gomp_fatal(). (see its handling in libgomp/target.c) In case OpenACC can have the same such zero-length array section behavior, we can just share one GOMP_MAP_ATTACH map. For now it is treated as separate cases. Chung-Lin > 2021-05-11 Julian Brown <julian@codesourcery.com> > > gcc/ > * gimplify.c (build_struct_comp_nodes): Add > GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION handling. > (build_struct_group): Process GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION > as part of pointer group. > (gimplify_scan_omp_clauses): Update prev_list_p such that > GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION will form part of pointer > group. > --- > gcc/gimplify.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/gcc/gimplify.c b/gcc/gimplify.c > index 6d204908c82..c5cb486aa23 100644 > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -8298,7 +8298,9 @@ build_struct_comp_nodes (enum tree_code code, tree grp_start, tree grp_end, > if (grp_mid > && OMP_CLAUSE_CODE (grp_mid) == OMP_CLAUSE_MAP > && (OMP_CLAUSE_MAP_KIND (grp_mid) == GOMP_MAP_ALWAYS_POINTER > - || OMP_CLAUSE_MAP_KIND (grp_mid) == GOMP_MAP_ATTACH_DETACH)) > + || OMP_CLAUSE_MAP_KIND (grp_mid) == GOMP_MAP_ATTACH_DETACH > + || (OMP_CLAUSE_MAP_KIND (grp_mid) > + == GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION))) > { > tree c3 > = build_omp_clause (OMP_CLAUSE_LOCATION (grp_end), OMP_CLAUSE_MAP); > @@ -8774,12 +8776,14 @@ build_struct_group (struct gimplify_omp_ctx *ctx, > ? splay_tree_lookup (ctx->variables, (splay_tree_key) decl) > : NULL); > bool ptr = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_POINTER); > - bool attach_detach = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH); > + bool attach_detach = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH > + || (OMP_CLAUSE_MAP_KIND (c) > + == GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION)); > bool attach = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH > || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH); > bool has_attachments = false; > /* For OpenACC, pointers in structs should trigger an attach action. */ > - if (attach_detach > + if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH > && ((region_type & (ORT_ACC | ORT_TARGET | ORT_TARGET_DATA)) > || code == OMP_TARGET_ENTER_DATA > || code == OMP_TARGET_EXIT_DATA)) > @@ -9784,6 +9788,8 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, > if (!remove > && OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_ALWAYS_POINTER > && OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_ATTACH_DETACH > + && (OMP_CLAUSE_MAP_KIND (c) > + != GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION) > && OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_TO_PSET > && OMP_CLAUSE_CHAIN (c) > && OMP_CLAUSE_CODE (OMP_CLAUSE_CHAIN (c)) == OMP_CLAUSE_MAP > @@ -9792,7 +9798,9 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, > || (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (c)) > == GOMP_MAP_ATTACH_DETACH) > || (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (c)) > - == GOMP_MAP_TO_PSET))) > + == GOMP_MAP_TO_PSET) > + || (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (c)) > + == GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION))) > prev_list_p = list_p; > > break; >
On Mon, 17 May 2021 21:14:26 +0800 Chung-Lin Tang <cltang@codesourcery.com> wrote: > On 2021/5/11 4:57 PM, Julian Brown wrote: > > This work-in-progress patch tries to get > > GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION to behave more like > > GOMP_MAP_ATTACH_DETACH -- in that the mapping is made to form groups > > to be processed by build_struct_group/build_struct_comp_map. I > > think that's important to integrate with how groups of mappings for > > array sections are handled in other cases. > > > > This patch isn't sufficient by itself to fix a couple of broken > > test cases at present (libgomp.c++/target-lambda-1.C, > > libgomp.c++/target-this-4.C), though. > > No, GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION is supposed to be just > a slightly different behavior version of GOMP_MAP_ATTACH; it > tolerates an unmapped pointer-target and assigns NULL on the device, > instead of just gomp_fatal(). (see its handling in libgomp/target.c) > > In case OpenACC can have the same such zero-length array section > behavior, we can just share one GOMP_MAP_ATTACH map. For now it is > treated as separate cases. 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! (The "prev_list_p" stuff in the loop in question in gimplify.c just keeps track of the first node in these groups.) 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. 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? 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? Thanks, Julian
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
diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 6d204908c82..c5cb486aa23 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -8298,7 +8298,9 @@ build_struct_comp_nodes (enum tree_code code, tree grp_start, tree grp_end, if (grp_mid && OMP_CLAUSE_CODE (grp_mid) == OMP_CLAUSE_MAP && (OMP_CLAUSE_MAP_KIND (grp_mid) == GOMP_MAP_ALWAYS_POINTER - || OMP_CLAUSE_MAP_KIND (grp_mid) == GOMP_MAP_ATTACH_DETACH)) + || OMP_CLAUSE_MAP_KIND (grp_mid) == GOMP_MAP_ATTACH_DETACH + || (OMP_CLAUSE_MAP_KIND (grp_mid) + == GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION))) { tree c3 = build_omp_clause (OMP_CLAUSE_LOCATION (grp_end), OMP_CLAUSE_MAP); @@ -8774,12 +8776,14 @@ build_struct_group (struct gimplify_omp_ctx *ctx, ? splay_tree_lookup (ctx->variables, (splay_tree_key) decl) : NULL); bool ptr = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_POINTER); - bool attach_detach = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH); + bool attach_detach = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH + || (OMP_CLAUSE_MAP_KIND (c) + == GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION)); bool attach = (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH); bool has_attachments = false; /* For OpenACC, pointers in structs should trigger an attach action. */ - if (attach_detach + if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH && ((region_type & (ORT_ACC | ORT_TARGET | ORT_TARGET_DATA)) || code == OMP_TARGET_ENTER_DATA || code == OMP_TARGET_EXIT_DATA)) @@ -9784,6 +9788,8 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, if (!remove && OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_ALWAYS_POINTER && OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_ATTACH_DETACH + && (OMP_CLAUSE_MAP_KIND (c) + != GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION) && OMP_CLAUSE_MAP_KIND (c) != GOMP_MAP_TO_PSET && OMP_CLAUSE_CHAIN (c) && OMP_CLAUSE_CODE (OMP_CLAUSE_CHAIN (c)) == OMP_CLAUSE_MAP @@ -9792,7 +9798,9 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, || (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (c)) == GOMP_MAP_ATTACH_DETACH) || (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (c)) - == GOMP_MAP_TO_PSET))) + == GOMP_MAP_TO_PSET) + || (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (c)) + == GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION))) prev_list_p = list_p; break;