Re: [PATCH 3/4] Factor out duplicate code in gimplify_scan_omp_clauses

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 16196 bytes --]

Hi Julian!

On 2019-10-06T15:32:36-0700, Julian Brown <julian@codesourcery.com> wrote:
> This patch factors out some code in gimplify_scan_omp_clauses into two
> new outlined functions.

Yay for such simplification, and yay for documenting what's going on!

> Previously approved here:
>
>   https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01309.html
>
> FAOD, OK for trunk?

I have not reviewed whether it's a suitable refactoring; I'm not at all
familiar with that code.

I started to "functionally" review it by re-inlining your outlined code,
and checking the differences to the original code.  Additionally to the
'gcc_assert' item I just raised with Jakub, there are a few more things I
don't understand:

> 	gcc/
> 	* gimplify.c (insert_struct_comp_map, check_base_and_compare_lt): New.
> 	(gimplify_scan_omp_clauses): Outline duplicated code into calls to
> 	above two functions.
> ---
>  gcc/gimplify.c | 307 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 174 insertions(+), 133 deletions(-)

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -8120,6 +8120,160 @@ gimplify_omp_depend (tree *list_p, gimple_seq *pre_p)
>    return 1;
>  }
>  
> +/* Insert a GOMP_MAP_ALLOC or GOMP_MAP_RELEASE node following a
> +   GOMP_MAP_STRUCT mapping.  C is an always_pointer mapping.  STRUCT_NODE is
> +   the struct node to insert the new mapping after (when the struct node is
> +   initially created).  PREV_NODE is the first of two or three mappings for a
> +   pointer, and is either:
> +     - the node before C, when a pair of mappings is used, e.g. for a C/C++
> +       array section.
> +     - not the node before C.  This is true when we have a reference-to-pointer
> +       type (with a mapping for the reference and for the pointer), or for
> +       Fortran derived-type mappings with a GOMP_MAP_TO_PSET.
> +   If SCP is non-null, the new node is inserted before *SCP.
> +   if SCP is null, the new node is inserted before PREV_NODE.
> +   The return type is:
> +     - PREV_NODE, if SCP is non-null.
> +     - The newly-created ALLOC or RELEASE node, if SCP is null.
> +     - The second newly-created ALLOC or RELEASE node, if we are mapping a
> +       reference to a pointer.  */
> +
> +static tree
> +insert_struct_comp_map (enum tree_code code, tree c, tree struct_node,
> +			tree prev_node, tree *scp)
> +{
> +  enum gomp_map_kind mkind
> +    = ((code == OMP_TARGET_EXIT_DATA || code == OACC_EXIT_DATA)
> +       ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC);

The original code does not handle 'OACC_EXIT_DATA', so that will be a
change separate from this refactoring?

> +
> +  tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
> +  tree cl = scp ? prev_node : c2;
> +  OMP_CLAUSE_SET_MAP_KIND (c2, mkind);
> +  OMP_CLAUSE_DECL (c2) = unshare_expr (OMP_CLAUSE_DECL (c));
> +  OMP_CLAUSE_CHAIN (c2) = scp ? *scp : prev_node;
> +  OMP_CLAUSE_SIZE (c2) = TYPE_SIZE_UNIT (ptr_type_node);
> +  if (struct_node)
> +    OMP_CLAUSE_CHAIN (struct_node) = c2;
> +
> +  /* We might need to create an additional mapping if we have a reference to a
> +     pointer (in C++).  Don't do this if we have something other than a
> +     GOMP_MAP_ALWAYS_POINTER though, i.e. a GOMP_MAP_TO_PSET.  */
> +  if (OMP_CLAUSE_CHAIN (prev_node) != c
> +      && OMP_CLAUSE_CODE (OMP_CLAUSE_CHAIN (prev_node)) == OMP_CLAUSE_MAP
> +      && (OMP_CLAUSE_MAP_KIND (OMP_CLAUSE_CHAIN (prev_node))
> +	  == GOMP_MAP_ALWAYS_POINTER))

In the original code, and with 'prev_node' being '*prev_list_p', this
condition was just 'if (OMP_CLAUSE_CHAIN (*prev_list_p) != c)', without
the 'GOMP_MAP_ALWAYS_POINTER' handling, so that'd be another change
separate from this refactoring?

> +    {
> +      tree c4 = OMP_CLAUSE_CHAIN (prev_node);
> +      tree c3 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
> +      OMP_CLAUSE_SET_MAP_KIND (c3, mkind);
> +      OMP_CLAUSE_DECL (c3) = unshare_expr (OMP_CLAUSE_DECL (c4));
> +      OMP_CLAUSE_SIZE (c3) = TYPE_SIZE_UNIT (ptr_type_node);
> +      OMP_CLAUSE_CHAIN (c3) = prev_node;
> +      if (!scp)
> +	OMP_CLAUSE_CHAIN (c2) = c3;
> +      else
> +	cl = c3;
> +    }
> +
> +  if (scp)
> +    *scp = c2;
> +
> +  return cl;
> +}
> +
> +/* Called initially with ORIG_BASE non-null, sets PREV_BITPOS and PREV_POFFSET
> +   to the offset of the field given in BASE.  Return type is 1 if BASE is equal
> +   to *ORIG_BASE after stripping off ARRAY_REF and INDIRECT_REF nodes and
> +   calling get_inner_reference, else 0.
> +
> +   Called subsequently with ORIG_BASE null, compares the offset of the field
> +   given in BASE to PREV_BITPOS, PREV_POFFSET. Returns -1 if the base object
> +   has changed, 0 if the new value has a higher bit position than that
> +   described by the aforementioned arguments, or 1 if the new value is less
> +   than them.  Used for (insertion) sorting components after a GOMP_MAP_STRUCT
> +   mapping.  */

Hmm, that doesn't seem to be the most intuitive API: the 'orig_base'
handling, specifically.  (See my struggle below.)

> +
> +static int
> +check_base_and_compare_lt (tree base, tree *orig_base, tree decl,
> +			   poly_int64 *prev_bitpos,
> +			   poly_offset_int *prev_poffset)
> +{
> +  tree offset;
> +  poly_int64 bitsize, bitpos;
> +  machine_mode mode;
> +  int unsignedp, reversep, volatilep = 0;
> +  poly_offset_int poffset;
> +
> +  if (orig_base)
> +    {
> +      while (TREE_CODE (base) == ARRAY_REF)
> +	base = TREE_OPERAND (base, 0);
> +
> +      if (TREE_CODE (base) == INDIRECT_REF)
> +	base = TREE_OPERAND (base, 0);
> +    }
> +  else
> +    {
> +      if (TREE_CODE (base) == ARRAY_REF)
> +	{
> +	  while (TREE_CODE (base) == ARRAY_REF)
> +	    base = TREE_OPERAND (base, 0);
> +	  if (TREE_CODE (base) != COMPONENT_REF
> +	      || TREE_CODE (TREE_TYPE (base)) != ARRAY_TYPE)
> +	    return -1;
> +	}
> +      else if (TREE_CODE (base) == INDIRECT_REF
> +	       && TREE_CODE (TREE_OPERAND (base, 0)) == COMPONENT_REF
> +	       && (TREE_CODE (TREE_TYPE (TREE_OPERAND (base, 0)))
> +		   == REFERENCE_TYPE))
> +	base = TREE_OPERAND (base, 0);
> +    }
> +
> +  base = get_inner_reference (base, &bitsize, &bitpos, &offset, &mode,
> +			      &unsignedp, &reversep, &volatilep);
> +
> +  if (orig_base)
> +    *orig_base = base;
> +
> +  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);
> +
> +  gcc_assert (offset == NULL_TREE || poly_int_tree_p (offset));
> +
> +  if (offset)
> +    poffset = wi::to_poly_offset (offset);
> +  else
> +    poffset = 0;
> +
> +  if (maybe_ne (bitpos, 0))
> +    poffset += bits_to_bytes_round_down (bitpos);
> +

I find the block of code following below until the end of the function a
bit unwieldy to (a) understand, and (b) relate to the original code:

> +  if (orig_base)
> +    {
> +      gcc_assert (base == decl);
> +
> +      *prev_bitpos = bitpos;
> +      *prev_poffset = poffset;
> +
> +      return *orig_base == base;
> +    }
> +  else
> +    {
> +      if (base != decl)
> +	return -1;
> +
> +      return (maybe_lt (*prev_poffset, poffset)
> +	      || (known_eq (*prev_poffset, poffset)
> +		  && maybe_lt (*prev_bitpos, bitpos)));
> +    }
> +
> +  return 0;
> +}

Can this be done differently, to make it easier to understand?  For
example, would it help if that code was not outlined into
'insert_struct_comp_map', but kept in its original places?  At least on
first sight that seemsa reasonable thing to do, given that it's just two
separate variants/code paths (as guarded by 'if (orig_base)'), where the
first of two calls takes the first branch ('orig_base' supplied), and the
other caller takes the second branch ('orig_base' not supplied).  If you
tell me that's not feasible, then I shall again try to understand that
code in the form you've proposed.


Grüße
 Thomas


> @@ -8660,29 +8814,14 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  			}
>  		    }
>  
> -		  tree offset;
> -		  poly_int64 bitsize, bitpos;
> -		  machine_mode mode;
> -		  int unsignedp, reversep, volatilep = 0;
> -		  tree base = OMP_CLAUSE_DECL (c);
> -		  while (TREE_CODE (base) == ARRAY_REF)
> -		    base = TREE_OPERAND (base, 0);
> -		  if (TREE_CODE (base) == INDIRECT_REF)
> -		    base = TREE_OPERAND (base, 0);
> -		  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))))
> -		      && DECL_P (TREE_OPERAND (base, 0))
> -		      && (TREE_CODE (TREE_TYPE (TREE_OPERAND (base, 0)))
> -			  == REFERENCE_TYPE))
> -		    base = TREE_OPERAND (base, 0);
> -		  gcc_assert (base == decl
> -			      && (offset == NULL_TREE
> -				  || poly_int_tree_p (offset)));
> +		  tree orig_base;
> +		  poly_int64 bitpos1;
> +		  poly_offset_int offset1;
> +
> +		  int base_eq_orig_base
> +		    = check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
> +						 &orig_base, decl, &bitpos1,
> +						 &offset1);
>  
>  		  splay_tree_node n
>  		    = splay_tree_lookup (ctx->variables, (splay_tree_key)decl);
> @@ -8693,7 +8832,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  		      tree l = build_omp_clause (OMP_CLAUSE_LOCATION (c),
>  						 OMP_CLAUSE_MAP);
>  		      OMP_CLAUSE_SET_MAP_KIND (l, GOMP_MAP_STRUCT);
> -		      if (orig_base != base)
> +		      if (!base_eq_orig_base)
>  			OMP_CLAUSE_DECL (l) = unshare_expr (orig_base);
>  		      else
>  			OMP_CLAUSE_DECL (l) = decl;
> @@ -8703,32 +8842,8 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  		      struct_map_to_clause->put (decl, l);
>  		      if (ptr)
>  			{
> -			  enum gomp_map_kind mkind
> -			    = code == OMP_TARGET_EXIT_DATA
> -			      ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC;
> -			  tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c),
> -						      OMP_CLAUSE_MAP);
> -			  OMP_CLAUSE_SET_MAP_KIND (c2, mkind);
> -			  OMP_CLAUSE_DECL (c2)
> -			    = unshare_expr (OMP_CLAUSE_DECL (c));
> -			  OMP_CLAUSE_CHAIN (c2) = *prev_list_p;
> -			  OMP_CLAUSE_SIZE (c2)
> -			    = TYPE_SIZE_UNIT (ptr_type_node);
> -			  OMP_CLAUSE_CHAIN (l) = c2;
> -			  if (OMP_CLAUSE_CHAIN (*prev_list_p) != c)
> -			    {
> -			      tree c4 = OMP_CLAUSE_CHAIN (*prev_list_p);
> -			      tree c3
> -				= build_omp_clause (OMP_CLAUSE_LOCATION (c),
> -						    OMP_CLAUSE_MAP);
> -			      OMP_CLAUSE_SET_MAP_KIND (c3, mkind);
> -			      OMP_CLAUSE_DECL (c3)
> -				= unshare_expr (OMP_CLAUSE_DECL (c4));
> -			      OMP_CLAUSE_SIZE (c3)
> -				= TYPE_SIZE_UNIT (ptr_type_node);
> -			      OMP_CLAUSE_CHAIN (c3) = *prev_list_p;
> -			      OMP_CLAUSE_CHAIN (c2) = c3;
> -			    }
> +			  insert_struct_comp_map (code, c, l, *prev_list_p,
> +						  NULL);
>  			  *prev_list_p = l;
>  			  prev_list_p = NULL;
>  			}
> @@ -8738,7 +8853,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  			  *list_p = l;
>  			  list_p = &OMP_CLAUSE_CHAIN (l);
>  			}
> -		      if (orig_base != base && code == OMP_TARGET)
> +		      if (!base_eq_orig_base && code == OMP_TARGET)
>  			{
>  			  tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c),
>  						      OMP_CLAUSE_MAP);
> @@ -8761,13 +8876,6 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  		      tree *sc = NULL, *scp = NULL;
>  		      if (GOMP_MAP_ALWAYS_P (OMP_CLAUSE_MAP_KIND (c)) || ptr)
>  			n->value |= GOVD_SEEN;
> -		      poly_offset_int o1, o2;
> -		      if (offset)
> -			o1 = wi::to_poly_offset (offset);
> -		      else
> -			o1 = 0;
> -		      if (maybe_ne (bitpos, 0))
> -			o1 += bits_to_bytes_round_down (bitpos);
>  		      sc = &OMP_CLAUSE_CHAIN (*osc);
>  		      if (*sc != c
>  			  && (OMP_CLAUSE_MAP_KIND (*sc)
> @@ -8785,44 +8893,14 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  			  break;
>  			else
>  			  {
> -			    tree offset2;
> -			    poly_int64 bitsize2, bitpos2;
> -			    base = OMP_CLAUSE_DECL (*sc);
> -			    if (TREE_CODE (base) == ARRAY_REF)
> -			      {
> -				while (TREE_CODE (base) == ARRAY_REF)
> -				  base = TREE_OPERAND (base, 0);
> -				if (TREE_CODE (base) != COMPONENT_REF
> -				    || (TREE_CODE (TREE_TYPE (base))
> -					!= ARRAY_TYPE))
> -				  break;
> -			      }
> -			    else if (TREE_CODE (base) == INDIRECT_REF
> -				     && (TREE_CODE (TREE_OPERAND (base, 0))
> -					 == COMPONENT_REF)
> -				     && (TREE_CODE (TREE_TYPE
> -						     (TREE_OPERAND (base, 0)))
> -					 == REFERENCE_TYPE))
> -			      base = TREE_OPERAND (base, 0);
> -			    base = get_inner_reference (base, &bitsize2,
> -							&bitpos2, &offset2,
> -							&mode, &unsignedp,
> -							&reversep, &volatilep);
> -			    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 != decl)
> +			    tree sc_decl = OMP_CLAUSE_DECL (*sc);
> +			    int same_decl_offset_lt
> +			      = check_base_and_compare_lt (sc_decl, NULL, decl,
> +							   &bitpos1, &offset1);
> +			    if (same_decl_offset_lt == -1)
>  			      break;
>  			    if (scp)
>  			      continue;
> -			    gcc_assert (offset == NULL_TREE
> -					|| poly_int_tree_p (offset));
>  			    tree d1 = OMP_CLAUSE_DECL (*sc);
>  			    tree d2 = OMP_CLAUSE_DECL (c);
>  			    while (TREE_CODE (d1) == ARRAY_REF)
> @@ -8851,14 +8929,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  				remove = true;
>  				break;
>  			      }
> -			    if (offset2)
> -			      o2 = wi::to_poly_offset (offset2);
> -			    else
> -			      o2 = 0;
> -			    o2 += bits_to_bytes_round_down (bitpos2);
> -			    if (maybe_lt (o1, o2)
> -				|| (known_eq (o1, o2)
> -				    && maybe_lt (bitpos, bitpos2)))
> +			    if (same_decl_offset_lt)
>  			      {
>  				if (ptr)
>  				  scp = sc;
> @@ -8873,38 +8944,8 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  				      size_one_node);
>  		      if (ptr)
>  			{
> -			  tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c),
> -						      OMP_CLAUSE_MAP);
> -			  tree cl = NULL_TREE;
> -			  enum gomp_map_kind mkind
> -			    = code == OMP_TARGET_EXIT_DATA
> -			      ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC;
> -			  OMP_CLAUSE_SET_MAP_KIND (c2, mkind);
> -			  OMP_CLAUSE_DECL (c2)
> -			    = unshare_expr (OMP_CLAUSE_DECL (c));
> -			  OMP_CLAUSE_CHAIN (c2) = scp ? *scp : *prev_list_p;
> -			  OMP_CLAUSE_SIZE (c2)
> -			    = TYPE_SIZE_UNIT (ptr_type_node);
> -			  cl = scp ? *prev_list_p : c2;
> -			  if (OMP_CLAUSE_CHAIN (*prev_list_p) != c)
> -			    {
> -			      tree c4 = OMP_CLAUSE_CHAIN (*prev_list_p);
> -			      tree c3
> -				= build_omp_clause (OMP_CLAUSE_LOCATION (c),
> -						    OMP_CLAUSE_MAP);
> -			      OMP_CLAUSE_SET_MAP_KIND (c3, mkind);
> -			      OMP_CLAUSE_DECL (c3)
> -				= unshare_expr (OMP_CLAUSE_DECL (c4));
> -			      OMP_CLAUSE_SIZE (c3)
> -				= TYPE_SIZE_UNIT (ptr_type_node);
> -			      OMP_CLAUSE_CHAIN (c3) = *prev_list_p;
> -			      if (!scp)
> -				OMP_CLAUSE_CHAIN (c2) = c3;
> -			      else
> -				cl = c3;
> -			    }
> -			  if (scp)
> -			    *scp = c2;
> +			  tree cl = insert_struct_comp_map (code, c, NULL,
> +							    *prev_list_p, scp);
>  			  if (sc == prev_list_p)
>  			    {
>  			      *sc = cl;


Grüße
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]