diff mbox

[gomp4] rewrite simd clone argument adjustment to avoid regimplification

Message ID 527AE0AF.2010106@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Nov. 7, 2013, 12:37 a.m. UTC
On 11/06/13 15:48, Jakub Jelinek wrote:
> 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.

Indeed!  Likewise for all my previous changes to ipa-prop.[ch] and 
tree-sra.c.

>> +  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.

Done.

>
>> +      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.

Done.

>
>> +  /* 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.

Hmmm, good point.  I've moved update_stmt and company to the caller, and 
modified the caller to call regimplify_operands only for GIMPLE_RETURN 
which is the special case.

Let me know if this is still ok so I can commit.

Thanks.
Aldy
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. 7, 2013, 7:38 a.m. UTC | #1
On Wed, Nov 06, 2013 at 05:37:03PM -0700, Aldy Hernandez wrote:
> Hmmm, good point.  I've moved update_stmt and company to the caller,
> and modified the caller to call regimplify_operands only for
> GIMPLE_RETURN which is the special case.

Can't you (later) handle that without regimplification too?
> 
> Let me know if this is still ok so I can commit.

Ok.

	Jakub
Martin Jambor Nov. 7, 2013, 5:58 p.m. UTC | #2
Hi,

On Wed, Nov 06, 2013 at 05:37:03PM -0700, Aldy Hernandez wrote:
> On 11/06/13 15:48, Jakub Jelinek wrote:
> >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.
> 
> Indeed!  Likewise for all my previous changes to ipa-prop.[ch] and
> tree-sra.c.

Sorry for the delay.  I'd just like to re-iterate one comment from my
previous email which is that I do not think tree-sra.c should export
any function to the outside world apart from the entry points of the
passes (yes, there is already build_ref_for_offset which I admit is
entirely my creation but that should be moved elswhere too).

So please put sra_ipa_get_adjustment_candidate to ipa-prop.c.  I see
that it requires get_ssa_base_param to be moved there as well but
since IPA-SRA uses it in different places, it would need exporting
too, which would be weird because it does not really do anything with
parameters.  Since the function is so trivial, I would even suggest
introducing another private copy to ipa-prop.c (and leaving the
original without the new parameter).  Alternatively, you can move the
function to tree.c but for that it looks too specialized.

Thanks,

Martin

> 
> >>+  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.
> 
> Done.
> 
> >
> >>+      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.
> 
> Done.
> 
> >
> >>+  /* 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.
> 
> Hmmm, good point.  I've moved update_stmt and company to the caller,
> and modified the caller to call regimplify_operands only for
> GIMPLE_RETURN which is the special case.
> 
> Let me know if this is still ok so I can commit.
> 
> Thanks.
> Aldy

> 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.
>
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 9d0ddcd..14b8571 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11049,33 +11049,75 @@  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, 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;
+      if (TYPE_P (*tp))
+	*walk_subtrees = 0;
+      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 (TREE_TYPE (t), 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;
+  info->modified = true;
+  wi->is_lhs = false;
+  wi->val_only = true;
+  return NULL_TREE;
 }
 
 /* Traverse the function body and perform all modifications as
@@ -11111,6 +11153,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 +11164,12 @@  ipa_simd_modify_function_body (struct cgraph_node *node,
       while (!gsi_end_p (gsi))
 	{
 	  gimple stmt = gsi_stmt (gsi);
-	  bool modified = false;
-	  tree *t;
-	  unsigned i;
+	  info.stmt = stmt;
+	  struct walk_stmt_info wi;
+
+	  memset (&wi, 0, sizeof (wi));
+	  info.modified = false;
+	  wi.info = &info;
 
 	  switch (gimple_code (stmt))
 	    {
@@ -11137,7 +11185,8 @@  ipa_simd_modify_function_body (struct cgraph_node *node,
 							NULL, NULL),
 						old_retval);
 		    gsi_replace (&gsi, stmt, true);
-		    modified = true;
+		    gimple_regimplify_operands (stmt, &gsi);
+		    info.modified = true;
 		  }
 		else
 		  {
@@ -11147,62 +11196,13 @@  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);
 	      break;
 	    }
 
-	  if (modified)
+	  if (info.modified)
 	    {
-	      gimple_regimplify_operands (stmt, &gsi);
 	      update_stmt (stmt);
 	      if (maybe_clean_eh_stmt (stmt))
 		gimple_purge_dead_eh_edges (gimple_bb (stmt));
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