[7/7,og10] WIP GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION changes

Message ID 04b90981e94acbabde004ee0992d404931e4fabf.1620721888.git.julian@codesourcery.com
State New
Headers show
Series
  • OpenACC/OpenMP: Rework struct component handling
Related show

Commit Message

Julian Brown May 11, 2021, 8:57 a.m.
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  <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(-)

-- 
2.29.2

Comments

Chung-Lin Tang May 17, 2021, 1:14 p.m. | #1
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;

>
Julian Brown May 17, 2021, 2:26 p.m. | #2
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
Chung-Lin Tang May 18, 2021, 11:13 a.m. | #3
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

Patch

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;