diff mbox

[gomp4] rewrite simd clone argument adjustment to avoid regimplification

Message ID 527AC1A8.70904@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Nov. 6, 2013, 10:24 p.m. UTC
On 11/04/13 08:44, Jakub Jelinek wrote:
> I guess short time yes, but I wonder if it wouldn't be better to use
> walk_gimple_op and do all changes in the callback.  Instead of passing
> adjustments pass around a struct containing the adjustments, current stmt
> and the modified flag.  You can use the val_only and is_lhs to determine
> what you need to do (probably need to reset those two for the subtrees
> to val_only = true, is_lhs = false and not walk subtrees of types) and you
> could (if not val_only) immediately gimplify it properly (insert temporary
> SSA_NAME setter before resp. store after depending on is_lhs).
> Then you could avoid the regimplification.

With the attached patch, we get rid of the ad-hoc argument adjustment 
system that is in place, and avoid regimplification altogether.  I am 
using walk_gimple_op as suggested, thus cleaning up most of 
ipa_simd_modify_function_body.

I have checked the following patch with the attached testcases that were 
previously ICEing, and with a handful of handcrafted tests that I 
checked manually (array references on lhs and rhs, vectors of pointers, 
etc).

OK for branch?
gcc/ChangeLog.gomp

	* ipa-prop.h (sra_ipa_get_adjustment_candidate): Protoize.
	* omp-low.c (struct modify_stmt_info): New.
	(ipa_simd_modify_function_body_ops_1): Remove.
	(ipa_simd_modify_stmt_ops): New.
	(ipa_simd_modify_function_body_ops): Remove.
	(ipa_simd_modify_function_body): Set new callback info.
	Remove special casing.  Handle all operators with walk_gimple_op.
	* tree-sra.c (get_ssa_base_param): Add new argument.  Use it.
	(disqualify_base_of_expr): Pass new argument to
	get_ssa_base_param.
	(sra_ipa_modify_expr): Abstract candidate search into...
	(sra_ipa_get_adjustment_candidate): ...here.

Comments

Jakub Jelinek Nov. 6, 2013, 10:48 p.m. UTC | #1
On Wed, Nov 06, 2013 at 03:24:40PM -0700, Aldy Hernandez wrote:
> I have checked the following patch with the attached testcases that
> were previously ICEing, and with a handful of handcrafted tests that
> I checked manually (array references on lhs and rhs, vectors of
> pointers, etc).

I'd like Martin to eyeball the ipa changes eventually.

> OK for branch?

Ok with a few nits fixed:

>  static tree
> -ipa_simd_modify_function_body_ops_1 (tree *tp, int *walk_subtrees, void *data)
> +ipa_simd_modify_stmt_ops (tree *tp,
> +			  int *walk_subtrees ATTRIBUTE_UNUSED,
> +			  void *data)
>  {
>    struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
> -  ipa_parm_adjustment_vec *adjustments = (ipa_parm_adjustment_vec *) wi->info;
> +  if (!SSA_VAR_P (*tp))
> +    {
> +      /* Make sure we treat subtrees as a RHS.  This makes sure that
> +	 when examining the `*foo' in *foo=x, the `foo' get treated as
> +	 a use properly.  */

I think it wouldn't hurt to e.g. do
	if (TYPE_P (*tp))
	  *walk_subtrees = 0;
here (and drop ATTRIBUTE_UNUSED above.

> +      wi->is_lhs = false;
> +      wi->val_only = true;
> +      return NULL_TREE;
> +    }
> +  struct modify_stmt_info *info = (struct modify_stmt_info *) wi->info;
> +  struct ipa_parm_adjustment *cand
> +    = sra_ipa_get_adjustment_candidate (tp, NULL, info->adjustments, true);
> +  if (!cand)
> +    return NULL_TREE;
> +
>    tree t = *tp;
> +  tree repl = make_ssa_name (create_tmp_var (TREE_TYPE (t), NULL), NULL);

Just do
  tree repl = make_ssa_name (TREE_TYPE (t), NULL);
no need to create underlying vars since 4.8.

> +  /* We have modified in place; update the SSA operands.  */
> +  info->modified = false;

So you always set modified to false?  I was expecting you'd set
it to true here and defer update_stmt and maybe_clean_eh_stmt etc.
to after walk_gimple_op (so, do it only when all the changes on the stmt
have been performed).  Plus, when you modify something, there is no need
to walk subtrees, so you can just do *walk_subtrees = 0; too.


	Jakub
diff mbox

Patch

diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 06296d1..6aebf8d 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -719,5 +719,8 @@  tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, tree,
 			   gimple_stmt_iterator *, bool);
 bool ipa_sra_modify_function_body (ipa_parm_adjustment_vec);
 bool sra_ipa_modify_expr (tree *, bool, ipa_parm_adjustment_vec);
+ipa_parm_adjustment *sra_ipa_get_adjustment_candidate (tree *&, bool *,
+						       ipa_parm_adjustment_vec,
+						       bool);
 
 #endif /* IPA_PROP_H */
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 94058af..0dd0676 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11049,33 +11049,81 @@  simd_clone_init_simd_arrays (struct cgraph_node *node,
   return seq;
 }
 
+/* Callback info for ipa_simd_modify_stmt_ops below.  */
+
+struct modify_stmt_info {
+  ipa_parm_adjustment_vec adjustments;
+  gimple stmt;
+  /* True if the parent statement was modified by
+     ipa_simd_modify_stmt_ops.  */
+  bool modified;
+};
+
+/* Callback for walk_gimple_op.
+
+   Adjust operands from a given statement as specified in the
+   adjustments vector in the callback data.  */
+
 static tree
-ipa_simd_modify_function_body_ops_1 (tree *tp, int *walk_subtrees, void *data)
+ipa_simd_modify_stmt_ops (tree *tp,
+			  int *walk_subtrees ATTRIBUTE_UNUSED,
+			  void *data)
 {
   struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
-  ipa_parm_adjustment_vec *adjustments = (ipa_parm_adjustment_vec *) wi->info;
+  if (!SSA_VAR_P (*tp))
+    {
+      /* Make sure we treat subtrees as a RHS.  This makes sure that
+	 when examining the `*foo' in *foo=x, the `foo' get treated as
+	 a use properly.  */
+      wi->is_lhs = false;
+      wi->val_only = true;
+      return NULL_TREE;
+    }
+  struct modify_stmt_info *info = (struct modify_stmt_info *) wi->info;
+  struct ipa_parm_adjustment *cand
+    = sra_ipa_get_adjustment_candidate (tp, NULL, info->adjustments, true);
+  if (!cand)
+    return NULL_TREE;
+
   tree t = *tp;
+  tree repl = make_ssa_name (create_tmp_var (TREE_TYPE (t), NULL), NULL);
 
-  if (DECL_P (t) || TREE_CODE (t) == SSA_NAME)
-    return (tree) sra_ipa_modify_expr (tp, true, *adjustments);
+  gimple stmt;
+  gimple_stmt_iterator gsi = gsi_for_stmt (info->stmt);
+  if (wi->is_lhs)
+    {
+      stmt = gimple_build_assign (unshare_expr (cand->reduction), repl);
+      gsi_insert_after (&gsi, stmt, GSI_SAME_STMT);
+      SSA_NAME_DEF_STMT (repl) = info->stmt;
+    }
   else
-    *walk_subtrees = 1;
-  return NULL_TREE;
-}
+    {
+      /* You'd think we could skip the extra SSA variable when
+	 wi->val_only=true, but we may have `*var' which will get
+	 replaced into `*var_array[iter]' and will likely be something
+	 not gimple.  */
+      stmt = gimple_build_assign (repl, unshare_expr (cand->reduction));
+      gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
+    }
 
-/* Helper function for ipa_simd_modify_function_body.  Make any
-   necessary adjustments for tree operators.  */
+  if (!useless_type_conversion_p (TREE_TYPE (*tp), TREE_TYPE (repl)))
+    {
+      tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*tp), repl);
+      *tp = vce;
+    }
+  else
+    *tp = repl;
 
-static bool
-ipa_simd_modify_function_body_ops (tree *tp,
-				   ipa_parm_adjustment_vec *adjustments)
-{
-  struct walk_stmt_info wi;
-  memset (&wi, 0, sizeof (wi));
-  wi.info = adjustments;
-  bool res = (bool) walk_tree (tp, ipa_simd_modify_function_body_ops_1,
-			       &wi, NULL);
-  return res;
+  /* We have modified in place; update the SSA operands.  */
+  info->modified = false;
+  stmt = info->stmt;
+  update_stmt (stmt);
+  if (maybe_clean_eh_stmt (stmt))
+    gimple_purge_dead_eh_edges (gimple_bb (stmt));
+
+  wi->is_lhs = false;
+  wi->val_only = true;
+  return NULL_TREE;
 }
 
 /* Traverse the function body and perform all modifications as
@@ -11111,6 +11159,9 @@  ipa_simd_modify_function_body (struct cgraph_node *node,
 		  NULL_TREE, NULL_TREE);
     }
 
+  struct modify_stmt_info info;
+  info.adjustments = adjustments;
+
   FOR_EACH_BB_FN (bb, DECL_STRUCT_FUNCTION (node->decl))
     {
       gimple_stmt_iterator gsi;
@@ -11119,9 +11170,13 @@  ipa_simd_modify_function_body (struct cgraph_node *node,
       while (!gsi_end_p (gsi))
 	{
 	  gimple stmt = gsi_stmt (gsi);
+	  info.stmt = stmt;
 	  bool modified = false;
-	  tree *t;
-	  unsigned i;
+	  struct walk_stmt_info wi;
+
+	  memset (&wi, 0, sizeof (wi));
+	  info.modified = false;
+	  wi.info = &info;
 
 	  switch (gimple_code (stmt))
 	    {
@@ -11147,56 +11202,9 @@  ipa_simd_modify_function_body (struct cgraph_node *node,
 		break;
 	      }
 
-	    case GIMPLE_ASSIGN:
-	      t = gimple_assign_lhs_ptr (stmt);
-	      modified |= sra_ipa_modify_expr (t, false, adjustments);
-
-	      /* The LHS may have operands that also need adjusting
-		 (e.g. `foo' in array[foo]).  */
-	      modified |= ipa_simd_modify_function_body_ops (t, &adjustments);
-
-	      for (i = 0; i < gimple_num_ops (stmt); ++i)
-		{
-		  t = gimple_op_ptr (stmt, i);
-		  modified |= sra_ipa_modify_expr (t, false, adjustments);
-		}
-	      break;
-
-	    case GIMPLE_CALL:
-	      /* Operands must be processed before the lhs.  */
-	      for (i = 0; i < gimple_call_num_args (stmt); i++)
-		{
-		  t = gimple_call_arg_ptr (stmt, i);
-		  modified |= sra_ipa_modify_expr (t, true, adjustments);
-		}
-
-	      if (gimple_call_lhs (stmt))
-		{
-		  t = gimple_call_lhs_ptr (stmt);
-		  modified |= sra_ipa_modify_expr (t, false, adjustments);
-		}
-	      break;
-
-	    case GIMPLE_ASM:
-	      for (i = 0; i < gimple_asm_ninputs (stmt); i++)
-		{
-		  t = &TREE_VALUE (gimple_asm_input_op (stmt, i));
-		  modified |= sra_ipa_modify_expr (t, true, adjustments);
-		}
-	      for (i = 0; i < gimple_asm_noutputs (stmt); i++)
-		{
-		  t = &TREE_VALUE (gimple_asm_output_op (stmt, i));
-		  modified |= sra_ipa_modify_expr (t, false, adjustments);
-		}
-	      break;
-
 	    default:
-	      for (i = 0; i < gimple_num_ops (stmt); ++i)
-		{
-		  t = gimple_op_ptr (stmt, i);
-		  if (*t)
-		    modified |= sra_ipa_modify_expr (t, true, adjustments);
-		}
+	      walk_gimple_op (stmt, ipa_simd_modify_stmt_ops, &wi);
+	      modified |= info.modified;
 	      break;
 	    }
 
diff --git a/gcc/testsuite/gcc.dg/gomp/simd-clones-7.c b/gcc/testsuite/gcc.dg/gomp/simd-clones-7.c
new file mode 100644
index 0000000..ef6fa11
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gomp/simd-clones-7.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-fopenmp -w" } */
+
+int array[1000];
+
+#pragma omp declare simd notinbranch simdlen(4)
+void foo (int *a, int b)
+{
+  a[b] = 555;
+}
+
+#pragma omp declare simd notinbranch simdlen(4)
+void bar (int *a)
+{
+  *a = 555;
+}
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 27938ab..36994f7 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -785,15 +785,17 @@  type_internals_preclude_sra_p (tree type, const char **msg)
     }
 }
 
-/* If T is an SSA_NAME, return NULL if it is not a default def or return its
-   base variable if it is.  Return T if it is not an SSA_NAME.  */
+/* If T is an SSA_NAME, return NULL if it is not a default def or
+   return its base variable if it is.  If IGNORE_DEFAULT_DEF is true,
+   the base variable is always returned, regardless if it is a default
+   def.  Return T if it is not an SSA_NAME.  */
 
 static tree
-get_ssa_base_param (tree t)
+get_ssa_base_param (tree t, bool ignore_default_def)
 {
   if (TREE_CODE (t) == SSA_NAME)
     {
-      if (SSA_NAME_IS_DEFAULT_DEF (t))
+      if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
 	return SSA_NAME_VAR (t);
       else
 	return NULL_TREE;
@@ -874,7 +876,7 @@  create_access (tree expr, gimple stmt, bool write)
   if (sra_mode == SRA_MODE_EARLY_IPA
       && TREE_CODE (base) == MEM_REF)
     {
-      base = get_ssa_base_param (TREE_OPERAND (base, 0));
+      base = get_ssa_base_param (TREE_OPERAND (base, 0), false);
       if (!base)
 	return NULL;
       ptr = true;
@@ -1039,7 +1041,7 @@  disqualify_base_of_expr (tree t, const char *reason)
   t = get_base_address (t);
   if (sra_mode == SRA_MODE_EARLY_IPA
       && TREE_CODE (t) == MEM_REF)
-    t = get_ssa_base_param (TREE_OPERAND (t, 0));
+    t = get_ssa_base_param (TREE_OPERAND (t, 0), false);
 
   if (t && DECL_P (t))
     disqualify_candidate (t, reason);
@@ -4485,35 +4487,35 @@  replace_removed_params_ssa_names (gimple stmt,
   return true;
 }
 
-/* If the expression *EXPR should be replaced by a reduction of a parameter, do
-   so.  ADJUSTMENTS is a pointer to a vector of adjustments.  CONVERT
-   specifies whether the function should care about type incompatibility the
-   current and new expressions.  If it is false, the function will leave
-   incompatibility issues to the caller.  Return true iff the expression
-   was modified. */
+/* Given an expression, return an adjustment entry specifying the
+   transformation to be done on EXPR.  If no suitable adjustment entry
+   was found, returns NULL.
 
-bool
-sra_ipa_modify_expr (tree *expr, bool convert,
-		     ipa_parm_adjustment_vec adjustments)
-{
-  int i, len;
-  struct ipa_parm_adjustment *adj, *cand = NULL;
-  HOST_WIDE_INT offset, size, max_size;
-  tree base, src;
+   If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
+   default def, otherwise bail on them.
 
-  len = adjustments.length ();
+   If CONVERT is non-NULL, this function will set *CONVERT if the
+   expression provided is a component reference that must be converted
+   upon return.  ADJUSTMENTS is the adjustments vector.  */
 
+ipa_parm_adjustment *
+sra_ipa_get_adjustment_candidate (tree *&expr, bool *convert,
+				  ipa_parm_adjustment_vec adjustments,
+				  bool ignore_default_def)
+{
   if (TREE_CODE (*expr) == BIT_FIELD_REF
       || TREE_CODE (*expr) == IMAGPART_EXPR
       || TREE_CODE (*expr) == REALPART_EXPR)
     {
       expr = &TREE_OPERAND (*expr, 0);
-      convert = true;
+      if (convert)
+	*convert = true;
     }
 
-  base = get_ref_base_and_extent (*expr, &offset, &size, &max_size);
+  HOST_WIDE_INT offset, size, max_size;
+  tree base = get_ref_base_and_extent (*expr, &offset, &size, &max_size);
   if (!base || size == -1 || max_size == -1)
-    return false;
+    return NULL;
 
   if (TREE_CODE (base) == MEM_REF)
     {
@@ -4521,13 +4523,15 @@  sra_ipa_modify_expr (tree *expr, bool convert,
       base = TREE_OPERAND (base, 0);
     }
 
-  base = get_ssa_base_param (base);
+  base = get_ssa_base_param (base, ignore_default_def);
   if (!base || TREE_CODE (base) != PARM_DECL)
-    return false;
+    return NULL;
 
-  for (i = 0; i < len; i++)
+  struct ipa_parm_adjustment *cand = NULL;
+  unsigned int len = adjustments.length ();
+  for (unsigned i = 0; i < len; i++)
     {
-      adj = &adjustments[i];
+      struct ipa_parm_adjustment *adj = &adjustments[i];
 
       if (adj->base == base
 	  && (adj->offset == offset || adj->remove_param))
@@ -4536,9 +4540,29 @@  sra_ipa_modify_expr (tree *expr, bool convert,
 	  break;
 	}
     }
+
   if (!cand || cand->copy_param || cand->remove_param)
+    return NULL;
+  return cand;
+}
+
+/* If the expression *EXPR should be replaced by a reduction of a parameter, do
+   so.  ADJUSTMENTS is a pointer to a vector of adjustments.  CONVERT
+   specifies whether the function should care about type incompatibility the
+   current and new expressions.  If it is false, the function will leave
+   incompatibility issues to the caller.  Return true iff the expression
+   was modified. */
+
+bool
+sra_ipa_modify_expr (tree *expr, bool convert,
+		     ipa_parm_adjustment_vec adjustments)
+{
+  struct ipa_parm_adjustment *cand
+    = sra_ipa_get_adjustment_candidate (expr, &convert, adjustments, false);
+  if (!cand)
     return false;
 
+  tree src;
   if (cand->by_ref)
     src = build_simple_mem_ref (cand->reduction);
   else