Re: [PATCH 7/7] [og10] WIP GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION changes
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
Re: [PATCH 7/7] [og10] WIP GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION changes
On Mon, 17 May 2021 21:14:26 +0800 Chung-Lin Tang 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
Re: [PATCH 7/7] [og10] WIP GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION changes
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 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;
[PATCH 7/7] [og10] WIP GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION changes
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. 2021-05-11 Julian Brown 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; -- 2.29.2