diff mbox series

[2/3] Factor out duplicate code in gimplify_scan_omp_clauses

Message ID e6bd7f517ae6fa2cedca649023934c836ef913f1.1541863637.git.julian@codesourcery.com
State New
Headers show
Series OpenACC 2.6 manual deep copy support (attach/detach) | expand

Commit Message

Julian Brown Nov. 10, 2018, 5:11 p.m. UTC 
This patch, created while trying to figure out the open-coded linked-list
handling in gimplify_scan_omp_clauses, factors out four somewhat
repetitive portions of that function into two new outlined functions.
This was done largely mechanically; the actual lines of executed code are
more-or-less the same.  That means the interfaces to the new functions
is somewhat eccentric though, and could no doubt be improved.  I've tried
to add commentary to the best of my understanding, but suggestions for
improvements are welcome!

As a bonus, one apparent bug introduced during an earlier refactoring
to use the polynomial types has been fixed (I think!): "known_eq (o1,
2)" should have been "known_eq (o1, o2)".

Tested alongside other patches in this series and the async patches. OK?

ChangeLog

	gcc/
	* gimplify.c (insert_struct_component_mapping)
	(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(-)

Comments

Julian Brown Dec. 18, 2018, 2:15 p.m. UTC | #1 
On Sat, 10 Nov 2018 09:11:19 -0800
Julian Brown <julian@codesourcery.com> wrote:

> This patch, created while trying to figure out the open-coded
> linked-list handling in gimplify_scan_omp_clauses, factors out four
> somewhat repetitive portions of that function into two new outlined
> functions. This was done largely mechanically; the actual lines of
> executed code are more-or-less the same.  That means the interfaces
> to the new functions is somewhat eccentric though, and could no doubt
> be improved.  I've tried to add commentary to the best of my
> understanding, but suggestions for improvements are welcome!
> 
> As a bonus, one apparent bug introduced during an earlier refactoring
> to use the polynomial types has been fixed (I think!): "known_eq (o1,
> 2)" should have been "known_eq (o1, o2)".
> 
> Tested alongside other patches in this series and the async patches.
> OK?

Now the main part of the attach/detach support has been conditionally
accepted pending Thomas's approval (thanks!), is this prerequisite part
OK too?

Thanks,

Julian
Jakub Jelinek Dec. 18, 2018, 2:50 p.m. UTC | #2 
On Sat, Nov 10, 2018 at 09:11:19AM -0800, Julian Brown wrote:
> This patch, created while trying to figure out the open-coded linked-list
> handling in gimplify_scan_omp_clauses, factors out four somewhat
> repetitive portions of that function into two new outlined functions.
> This was done largely mechanically; the actual lines of executed code are
> more-or-less the same.  That means the interfaces to the new functions
> is somewhat eccentric though, and could no doubt be improved.  I've tried
> to add commentary to the best of my understanding, but suggestions for
> improvements are welcome!
> 
> As a bonus, one apparent bug introduced during an earlier refactoring
> to use the polynomial types has been fixed (I think!): "known_eq (o1,
> 2)" should have been "known_eq (o1, o2)".
> 
> Tested alongside other patches in this series and the async patches. OK?
> 
> ChangeLog
> 
> 	gcc/
> 	* gimplify.c (insert_struct_component_mapping)
> 	(check_base_and_compare_lt): New.

I think
	* gimplify.c (insert_struct_component_mapping,
	check_base_and_compare_lt): New.
is what is used far more often than the above syntax.

> +
> +static tree
> +insert_struct_component_mapping (enum tree_code code, tree c, tree struct_node,
> +				 tree prev_node, tree *scp)

Please use a shorter name, like insert_struct_comp_mapping or even
insert_struct_comp_map, to avoid formatting glitches.

> +{
> +  enum gomp_map_kind mkind = (code == OMP_TARGET_EXIT_DATA
> +			      || code == OACC_EXIT_DATA)
> +			     ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC;

Please use
  enum gomp_map_kind mkind
    = ((code == OMP_TARGET_EXIT_DATA || code == OACC_EXIT_DATA)
       ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC);
instead.

> +		  int base_eq_orig_base
> +		    = check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
> +			&orig_base, decl, &bitpos1, &offset1);

Incorrect formatting, &orig_base needs to be below OMP_CLAUSE_DECL.  So:
		  int base_eq_orig_base
		    = check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
						 &orig_base, decl, &bitpos1,
						 &offset1);

> +			    int same_decl_offset_lt
> +			      = check_base_and_compare_lt (
> +				  OMP_CLAUSE_DECL (*sc), NULL, decl,
> +				  &bitpos1, &offset1);
> +			    if (same_decl_offset_lt == -1)

Again, wrong formatting.  If even the first argument doesn't fit, just use
a temporary.
			    tree sc_decl = OMP_CLAUSE_DECL (*sc);
			    int same_decl_offset_lt
			      = check_base_and_compare_lt (sc_decl, NULL, decl,
							   &bitpos1, &offset1);

> +			  tree cl
> +			    = insert_struct_component_mapping (code, c, NULL,
> +				*prev_list_p, scp);

Also wrong formatting, should be:

			  tree cl
			    = insert_struct_component_mapping (code, c, NULL,
							       *prev_list_p,
							       scp);

or if the name is shorter, you can fit more.

>  			  if (sc == prev_list_p)
>  			    {
>  			      *sc = cl;

Otherwise LGTM, but I admit I haven't verified every single statement.

	Jakub
diff mbox series

Patch

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 61dca24..274edc0 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -7967,6 +7967,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_component_mapping (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;
+
+  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))
+    {
+      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.  */
+
+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);
+
+  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;
+}
+
 /* Scan the OMP clauses in *LIST_P, installing mappings into a new
    and previous omp contexts.  */
 
@@ -8474,29 +8628,13 @@  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);
@@ -8507,7 +8645,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;
@@ -8517,32 +8655,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_component_mapping (code, c, l,
+							   *prev_list_p, NULL);
 			  *prev_list_p = l;
 			  prev_list_p = NULL;
 			}
@@ -8552,7 +8666,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);
@@ -8575,13 +8689,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)
@@ -8599,44 +8706,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)
+			    int same_decl_offset_lt
+			      = check_base_and_compare_lt (
+				  OMP_CLAUSE_DECL (*sc), 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)
@@ -8665,14 +8742,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, 2)
-				    && maybe_lt (bitpos, bitpos2)))
+			    if (same_decl_offset_lt)
 			      {
 				if (ptr)
 				  scp = sc;
@@ -8687,38 +8757,9 @@  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_component_mapping (code, c, NULL,
+				*prev_list_p, scp);
 			  if (sc == prev_list_p)
 			    {
 			      *sc = cl;