Re: [PATCH 4/5] Rework indirect struct handling for OpenACC/OpenMP in gimplify.c

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 5/14/21 11:26 PM, Julian Brown wrote:
> This patch reworks indirect struct handling in gimplify.c (i.e. for struct
> components mapped with "mystruct->a[0:n]", "mystruct->b", etc.), for
> both OpenACC and OpenMP.  The key observation leading to these changes
> was that component mappings of references-to-structures is already
> implemented and working, and indirect struct component handling via a
> pointer can work quite similarly.  That lets us remove some earlier,
> special-case handling for mapping indirect struct component accesses
> for OpenACC, which required the pointed-to struct to be manually mapped
> before the indirect component mapping.
> 
> With this patch, you can map struct components directly (e.g. an array
> slice "mystruct->a[0:n]") just like you can map a non-indirect struct
> component slice ("mystruct.a[0:n]"). Both references-to-pointers (with
> the former syntax) and references to structs (with the latter syntax)
> work now.
> 
> For Fortran class pointers, we no longer re-use GOMP_MAP_TO_PSET for the
> class metadata (the structure that points to the class data and vptr)
> -- it is instead treated as any other struct.
> 
> For C++, the struct handling also works for class members ("this->foo"),
> without having to explicitly map "this[:1]" first.
> 
> For OpenACC, we permit chained indirect component references
> ("mystruct->a->b[0:n]"), though only the last part of such mappings will
> trigger an attach/detach operation.  To properly use such a construct
> on the target, you must still manually map "mystruct->a[:1]" first --
> but there's no need to map "mystruct[:1]" explicitly before that.
> 
> This patch incorporates parts of Chung-Lin's patch "Recommit "Enable
> gimplify GOMP_MAP_STRUCT handling of (COMPONENT_REF (INDIRECT_REF
> ...)) map clauses"." from the og10 branch.
> 
> OK for trunk?
> 
> Thanks,
> 
> Julian
> 
> 2021-05-14  Julian Brown  <julian@codesourcery.com>
> 	    Chung-Lin Tang  <cltang@codesourcery.com>
> 
> gcc/fortran/
> 	* trans-openmp.c (gfc_trans_omp_clauses): Don't create GOMP_MAP_TO_PSET
> 	mappings for class metadata, nor GOMP_MAP_POINTER mappings for
> 	POINTER_TYPE_P decls.
> 
> gcc/
> 	* gimplify.c (tree-hash-traits.h): Include.
> 	(extract_base_bit_offset): Add BASE_IND parameter.  Handle
> 	pointer-typed indirect references alongside reference-typed ones.
> 	(strip_components_and_deref, aggregate_base_p): New functions.
> 	(build_struct_group): Update struct_map_to_clause type.  Add pointer
> 	type indirect ref handling, including chained references.  Handle
> 	pointers and references to structs in OpenACC regions as well as
> 	OpenMP ones.
> 	(gimplify_scan_omp_clauses): Remove struct_deref_set handling.  Rework
> 	pointer-type indirect structure access handling to work more like
> 	the reference-typed handling.
> 	* omp-low.c (scan_sharing_clauses): Handle pointer-type indirect struct
> 	references, and references to pointers to structs also.
> 
> gcc/testsuite/
> 	* g++.dg/goacc/member-array-acc.C: New test (XFAILed for now).
> 	* g++.dg/gomp/member-array-omp.C: New test (XFAILed for now).
> 
> libgomp/
> 	* testsuite/libgomp.oacc-c-c++-common/deep-copy-15.c: New test.
> 	* testsuite/libgomp.oacc-c-c++-common/deep-copy-16.c: New test.
> 	* testsuite/libgomp.oacc-c++/deep-copy-17.C: New test.
> ---
>  gcc/fortran/trans-openmp.c                    |  20 +-
>  gcc/gimplify.c                                | 285 ++++++++++--------
>  gcc/omp-low.c                                 |  16 +-
>  gcc/testsuite/g++.dg/goacc/member-array-acc.C |  14 +
>  gcc/testsuite/g++.dg/gomp/member-array-omp.C  |  14 +
>  .../testsuite/libgomp.oacc-c++/deep-copy-17.C | 101 +++++++
>  .../libgomp.oacc-c-c++-common/deep-copy-15.c  |  71 +++++
>  .../libgomp.oacc-c-c++-common/deep-copy-16.c  | 231 ++++++++++++++
>  8 files changed, 612 insertions(+), 140 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/goacc/member-array-acc.C
>  create mode 100644 gcc/testsuite/g++.dg/gomp/member-array-omp.C
>  create mode 100644 libgomp/testsuite/libgomp.oacc-c++/deep-copy-17.C
>  create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/deep-copy-15.c
>  create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/deep-copy-16.c
> 
> diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
> index 5666cd68c7e..ff614ffe744 100644
> --- a/gcc/fortran/trans-openmp.c
> +++ b/gcc/fortran/trans-openmp.c
> @@ -2721,30 +2721,16 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
>  		  tree present = gfc_omp_check_optional_argument (decl, true);
>  		  if (openacc && n->sym->ts.type == BT_CLASS)
>  		    {
> -		      tree type = TREE_TYPE (decl);
>  		      if (n->sym->attr.optional)
>  			sorry ("optional class parameter");
> -		      if (POINTER_TYPE_P (type))
> -			{
> -			  node4 = build_omp_clause (input_location,
> -						    OMP_CLAUSE_MAP);
> -			  OMP_CLAUSE_SET_MAP_KIND (node4, GOMP_MAP_POINTER);
> -			  OMP_CLAUSE_DECL (node4) = decl;
> -			  OMP_CLAUSE_SIZE (node4) = size_int (0);
> -			  decl = build_fold_indirect_ref (decl);
> -			}
>  		      tree ptr = gfc_class_data_get (decl);
>  		      ptr = build_fold_indirect_ref (ptr);
>  		      OMP_CLAUSE_DECL (node) = ptr;
>  		      OMP_CLAUSE_SIZE (node) = gfc_class_vtab_size_get (decl);
>  		      node2 = build_omp_clause (input_location, OMP_CLAUSE_MAP);
> -		      OMP_CLAUSE_SET_MAP_KIND (node2, GOMP_MAP_TO_PSET);
> -		      OMP_CLAUSE_DECL (node2) = decl;
> -		      OMP_CLAUSE_SIZE (node2) = TYPE_SIZE_UNIT (type);
> -		      node3 = build_omp_clause (input_location, OMP_CLAUSE_MAP);
> -		      OMP_CLAUSE_SET_MAP_KIND (node3, GOMP_MAP_ATTACH_DETACH);
> -		      OMP_CLAUSE_DECL (node3) = gfc_class_data_get (decl);
> -		      OMP_CLAUSE_SIZE (node3) = size_int (0);
> +		      OMP_CLAUSE_SET_MAP_KIND (node2, GOMP_MAP_ATTACH_DETACH);
> +		      OMP_CLAUSE_DECL (node2) = gfc_class_data_get (decl);
> +		      OMP_CLAUSE_SIZE (node2) = size_int (0);
>  		      goto finalize_map_clause;
>  		    }
>  		  else if (POINTER_TYPE_P (TREE_TYPE (decl))
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 69ab637367c..c0f068a725d 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "langhooks.h"
>  #include "tree-cfg.h"
>  #include "tree-ssa.h"
> +#include "tree-hash-traits.h"
>  #include "omp-general.h"
>  #include "omp-low.h"
>  #include "gimple-low.h"
> @@ -8351,8 +8352,8 @@ build_struct_comp_nodes (enum tree_code code, tree grp_start, tree grp_end,
>     has array type, else return NULL.  */
>  
>  static tree
> -extract_base_bit_offset (tree base, tree *base_ref, poly_int64 *bitposp,
> -			 poly_offset_int *poffsetp)
> +extract_base_bit_offset (tree base, tree *base_ind, tree *base_ref,
> +			 poly_int64 *bitposp, poly_offset_int *poffsetp)
>  {
>    tree offset;
>    poly_int64 bitsize, bitpos;
> @@ -8360,6 +8361,9 @@ extract_base_bit_offset (tree base, tree *base_ref, poly_int64 *bitposp,
>    int unsignedp, reversep, volatilep = 0;
>    poly_offset_int poffset;
>  
> +  if (base_ind)
> +    *base_ind = NULL_TREE;
> +
>    if (base_ref)
>      *base_ref = NULL_TREE;
>  
> @@ -8380,14 +8384,27 @@ extract_base_bit_offset (tree base, tree *base_ref, poly_int64 *bitposp,
>    base = get_inner_reference (base, &bitsize, &bitpos, &offset, &mode,
>  			      &unsignedp, &reversep, &volatilep);
>  
> -  tree orig_base = base;
> -
> +  if ((TREE_CODE (base) == INDIRECT_REF
> +       || (TREE_CODE (base) == MEM_REF
> +	   && integer_zerop (TREE_OPERAND (base, 1))))
> +      && TREE_CODE (TREE_TYPE (TREE_OPERAND (base, 0))) == POINTER_TYPE)
> +    {
> +      if (base_ind)
> +	*base_ind = base;
> +      base = TREE_OPERAND (base, 0);
> +    }
>    if ((TREE_CODE (base) == INDIRECT_REF
>         || (TREE_CODE (base) == MEM_REF
>  	   && integer_zerop (TREE_OPERAND (base, 1))))
>        && DECL_P (TREE_OPERAND (base, 0))
>        && TREE_CODE (TREE_TYPE (TREE_OPERAND (base, 0))) == REFERENCE_TYPE)
> -    base = TREE_OPERAND (base, 0);
> +    {
> +      if (base_ref)
> +	*base_ref = base;
> +      base = TREE_OPERAND (base, 0);
> +    }
> +
> +  STRIP_NOPS (base);
>  
>    gcc_assert (offset == NULL_TREE || poly_int_tree_p (offset));
>  
> @@ -8402,10 +8419,6 @@ extract_base_bit_offset (tree base, tree *base_ref, poly_int64 *bitposp,
>    *bitposp = bitpos;
>    *poffsetp = poffset;
>  
> -  /* Set *BASE_REF if BASE was a dereferenced reference variable.  */
> -  if (base_ref && orig_base != base)
> -    *base_ref = orig_base;
> -
>    return base;
>  }
>  
> @@ -8422,6 +8435,48 @@ is_or_contains_p (tree expr, tree base_ptr)
>    return expr == base_ptr;
>  }
>  
> +/* Remove COMPONENT_REFS and indirections from EXPR.  */
> +
> +static tree
> +strip_components_and_deref (tree expr)
> +{
> +  while (TREE_CODE (expr) == COMPONENT_REF
> +	 || TREE_CODE (expr) == INDIRECT_REF
> +	 || (TREE_CODE (expr) == MEM_REF
> +	     && integer_zerop (TREE_OPERAND (expr, 1))))
> +    expr = TREE_OPERAND (expr, 0);
> +
> +  return expr;
> +}
> +
> +/* Return TRUE if EXPR is something we will use as the base of an aggregate
> +   access, either:
> +
> +  - a DECL_P.
> +  - a struct component with no indirection ("a.b.c").
> +  - a struct component with indirection ("a->b->c").
> +*/
> +
> +static bool
> +aggregate_base_p (tree expr)
> +{
> +  while (TREE_CODE (expr) == COMPONENT_REF
> +	 && (DECL_P (TREE_OPERAND (expr, 0))
> +	     || (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPONENT_REF)))
> +    expr = TREE_OPERAND (expr, 0);
> +
> +  if (DECL_P (expr))
> +    return true;
> +
> +  if (TREE_CODE (expr) == COMPONENT_REF
> +      && (TREE_CODE (TREE_OPERAND (expr, 0)) == INDIRECT_REF
> +	  || (TREE_CODE (TREE_OPERAND (expr, 0)) == MEM_REF
> +	      && integer_zerop (TREE_OPERAND (TREE_OPERAND (expr, 0), 1)))))
> +    return true;
> +
> +  return false;
> +}
> +
>  /* Implement OpenMP 5.x map ordering rules for target directives. There are
>     several rules, and with some level of ambiguity, hopefully we can at least
>     collect the complexity here in one place.  */
> @@ -8715,19 +8770,26 @@ static tree
>  build_struct_group (struct gimplify_omp_ctx *ctx,
>  		    enum omp_region_type region_type, enum tree_code code,
>  		    tree decl, unsigned int *flags, tree c,
> -		    hash_map<tree, tree> *&struct_map_to_clause,
> +		    hash_map<tree_operand_hash, tree> *&struct_map_to_clause,
>  		    tree *&prev_list_p, tree *&list_p, bool *cont)
>  {
>    poly_offset_int coffset;
>    poly_int64 cbitpos;
> -  tree base_ref;
> +  tree base_ind, base_ref;
> +  tree *list_in_p = list_p, *prev_list_in_p = prev_list_p;
>  

Is this a kind of debug code?
This fails to compile:

../../gcc-trunk/gcc/gimplify.c: In function ‘tree_node* build_struct_group(gimplify_omp_ctx*, omp_region_type, tree_code, tree, unsigned int*, tree, hash_map<tree_operand_hash, tree_node*>*&, tree_node**&, tree_node**&, bool*)’:
../../gcc-trunk/gcc/gimplify.c:8779:9: error: unused variable ‘list_in_p’ [-Werror=unused-variable]
 8779 |   tree *list_in_p = list_p, *prev_list_in_p = prev_list_p;
      |         ^~~~~~~~~
../../gcc-trunk/gcc/gimplify.c:8779:30: error: unused variable ‘prev_list_in_p’ [-Werror=unused-variable]
 8779 |   tree *list_in_p = list_p, *prev_list_in_p = prev_list_p;
      |                              ^~~~~~~~~~~~~~


You need to boot-strap and do your regression testing before sending
a patch to this list.


Thanks
Bernd.