[omp4]

[omp4]

[Aldy Hernandez]

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

Hi.

While looking over some of your testcases I noticed that array 
subscripts are not being properly adjusted:

foo(int i) {
	array[i] = ....
}

The `i' should use the simd array magic instead of ICEing :).

Is the attached patch OK for the branch?

Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 2443 bytes --]

gcc/ChangeLog.gomp

	* omp-low.c (ipa_simd_modify_function_body): Adjust tree operands
	on the LHS of an assign.
	(ipa_simd_modify_function_body_ops_1): New.
	(ipa_simd_modify_function_body_ops): New.

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index d30fb17..94058af 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11049,6 +11049,35 @@ simd_clone_init_simd_arrays (struct cgraph_node *node,
   return seq;
 }
 
+static tree
+ipa_simd_modify_function_body_ops_1 (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;
+  tree t = *tp;
+
+  if (DECL_P (t) || TREE_CODE (t) == SSA_NAME)
+    return (tree) sra_ipa_modify_expr (tp, true, *adjustments);
+  else
+    *walk_subtrees = 1;
+  return NULL_TREE;
+}
+
+/* Helper function for ipa_simd_modify_function_body.  Make any
+   necessary adjustments for tree operators.  */
+
+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;
+}
+
 /* Traverse the function body and perform all modifications as
    described in ADJUSTMENTS.  At function return, ADJUSTMENTS will be
    modified such that the replacement/reduction value will now be an
@@ -11121,6 +11150,11 @@ ipa_simd_modify_function_body (struct cgraph_node *node,
 	    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);
diff --git a/gcc/testsuite/gcc.dg/gomp/simd-clones-6.c b/gcc/testsuite/gcc.dg/gomp/simd-clones-6.c
new file mode 100644
index 0000000..8818594
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gomp/simd-clones-6.c
@@ -0,0 +1,11 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-fopenmp" } */
+
+/* Test that array subscripts are properly adjusted.  */
+
+int array[1000];
+#pragma omp declare simd notinbranch simdlen(4)
+void foo (int i)
+{
+  array[i] = 555;
+}

Re: [omp4] fix array subscripts in simd clones

[Aldy Hernandez]

On 11/04/13 08:19, Aldy Hernandez wrote:
> Hi.
>
> While looking over some of your testcases I noticed that array
> subscripts are not being properly adjusted:
>
> foo(int i) {
>      array[i] = ....
> }
>
> The `i' should use the simd array magic instead of ICEing :).
>
> Is the attached patch OK for the branch?
>
> Aldy

Ughh, my apologies for the lack of subject.  Fixed in this followup.

Re: [omp4]

[Jakub Jelinek]

On Mon, Nov 04, 2013 at 08:19:12AM -0700, Aldy Hernandez wrote:
> Hi.
> 
> While looking over some of your testcases I noticed that array
> subscripts are not being properly adjusted:
> 
> foo(int i) {
> 	array[i] = ....
> }
> 
> The `i' should use the simd array magic instead of ICEing :).
> 
> Is the attached patch OK for the branch?

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.

	Jakub

Re: [omp4]

[Aldy Hernandez]

On 11/04/13 08:44, Jakub Jelinek wrote:
> On Mon, Nov 04, 2013 at 08:19:12AM -0700, Aldy Hernandez wrote:
>> Hi.
>>
>> While looking over some of your testcases I noticed that array
>> subscripts are not being properly adjusted:
>>
>> foo(int i) {
>> 	array[i] = ....
>> }
>>
>> The `i' should use the simd array magic instead of ICEing :).
>>
>> Is the attached patch OK for the branch?
>
> 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.
>

You mean rewrite the entire ipa_simd_modify_function_body() with 
walk_gimple_op and friends, or just the bits I suggested with the above 
patch (the LHS operands in a GIMPLE_ASSIGN)?

Aldy

Re: [omp4]

[Jakub Jelinek]

On Mon, Nov 04, 2013 at 09:12:10AM -0700, Aldy Hernandez wrote:
> On 11/04/13 08:44, Jakub Jelinek wrote:
> >On Mon, Nov 04, 2013 at 08:19:12AM -0700, Aldy Hernandez wrote:
> >>Hi.
> >>
> >>While looking over some of your testcases I noticed that array
> >>subscripts are not being properly adjusted:
> >>
> >>foo(int i) {
> >>	array[i] = ....
> >>}
> >>
> >>The `i' should use the simd array magic instead of ICEing :).
> >>
> >>Is the attached patch OK for the branch?
> >
> >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.
> >
> 
> You mean rewrite the entire ipa_simd_modify_function_body() with
> walk_gimple_op and friends, or just the bits I suggested with the
> above patch (the LHS operands in a GIMPLE_ASSIGN)?

Most of the ipa_simd_modify_function_body function, yes.
Perhaps you can't easily handle that way say GIMPLE_RETURN or something, but
generally for other random statements I don't see why it couldn't work
easily.

	Jakub

Re: [omp4]

[Aldy Hernandez]

On 11/04/13 09:16, Jakub Jelinek wrote:
> On Mon, Nov 04, 2013 at 09:12:10AM -0700, Aldy Hernandez wrote:
>> On 11/04/13 08:44, Jakub Jelinek wrote:
>>> On Mon, Nov 04, 2013 at 08:19:12AM -0700, Aldy Hernandez wrote:
>>>> Hi.
>>>>
>>>> While looking over some of your testcases I noticed that array
>>>> subscripts are not being properly adjusted:
>>>>
>>>> foo(int i) {
>>>> 	array[i] = ....
>>>> }
>>>>
>>>> The `i' should use the simd array magic instead of ICEing :).
>>>>
>>>> Is the attached patch OK for the branch?
>>>
>>> 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.
>>>
>>
>> You mean rewrite the entire ipa_simd_modify_function_body() with
>> walk_gimple_op and friends, or just the bits I suggested with the
>> above patch (the LHS operands in a GIMPLE_ASSIGN)?
>
> Most of the ipa_simd_modify_function_body function, yes.
> Perhaps you can't easily handle that way say GIMPLE_RETURN or something, but
> generally for other random statements I don't see why it couldn't work
> easily.

Ok, then let's leave this for later.  I'm following what we've done in 
tree-sra.c to be consistent.  Perhaps later we could even clean up 
ipa_sra_modify_function_body in tree-sra.c.  But for now I think we can 
concentrate on the basic functionality.

Committing to omp4 branch.

Re: [omp4]

[Jakub Jelinek]

On Mon, Nov 04, 2013 at 09:22:39AM -0700, Aldy Hernandez wrote:
> Ok, then let's leave this for later.  I'm following what we've done
> in tree-sra.c to be consistent.  Perhaps later we could even clean
> up ipa_sra_modify_function_body in tree-sra.c.  But for now I think
> we can concentrate on the basic functionality.

Well, ipa_sra_modify_function_body doesn't need to regimplify, unlike
the omp-low.c version thereof (supposedly because SRA only modifies
loads/stores, but doesn't change SSA_NAMEs on arithmetics etc.).
But yeah, this isn't requirement for hacking on this on gomp-4_0-branch, but
it would be nice to clean it up before merging to trunk.

> Committing to omp4 branch.

Thanks.

	Jakub

[gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Aldy Hernandez]

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

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?

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 12520 bytes --]

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

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Jakub Jelinek]

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

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Aldy Hernandez]

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

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

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 12783 bytes --]

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

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Jakub Jelinek]

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

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Aldy Hernandez]

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

On 11/07/13 00:38, Jakub Jelinek wrote:
> 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?

Sure!  See attached patch.

But as discussed on IRC, I wonder whether we can do without the 
following in the attached patch:

+		    tree repl = make_ssa_name (TREE_TYPE (retval), NULL);
+		    stmt = gimple_build_assign (repl, retval);
+		    gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
+		    stmt = gimple_build_assign (ref, repl);

...and unconditionally do:

+		  stmt = gimple_build_assign (ref, retval);

...since it seems all the GIMPLE_RETURNs I see can be replaced by 
``retval_array[iter] = whatever'' without creating something non-gimple 
(thus avoiding an SSA variable).

Either way, I'm ok.  Let me know.

Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 1592 bytes --]

gcc/ChangeLog.gomp

	* omp-low.c (ipa_simd_modify_function_body): Avoid
	regimplification of GIMPLE_RETURNs.

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 14b8571..6039de9 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11175,26 +11175,31 @@ ipa_simd_modify_function_body (struct cgraph_node *node,
 	    {
 	    case GIMPLE_RETURN:
 	      {
-		tree old_retval = gimple_return_retval (stmt);
-		if (old_retval)
+		tree retval = gimple_return_retval (stmt);
+		if (!retval)
 		  {
-		    /* Replace `return foo' by `retval_array[iter] = foo'.  */
-		    stmt = gimple_build_assign (build4 (ARRAY_REF,
-							TREE_TYPE (old_retval),
-							retval_array, iter,
-							NULL, NULL),
-						old_retval);
-		    gsi_replace (&gsi, stmt, true);
-		    gimple_regimplify_operands (stmt, &gsi);
-		    info.modified = true;
+		    gsi_remove (&gsi, true);
+		    continue;
 		  }
+
+		/* Replace `return foo' with `retval_array[iter] = foo'.  */
+		tree ref = build4 (ARRAY_REF,
+				   TREE_TYPE (retval),
+				   retval_array, iter,
+				   NULL, NULL);
+		if (is_gimple_reg (retval))
+		  stmt = gimple_build_assign (ref, retval);
 		else
 		  {
-		    gsi_remove (&gsi, true);
-		    continue;
+		    tree repl = make_ssa_name (TREE_TYPE (retval), NULL);
+		    stmt = gimple_build_assign (repl, retval);
+		    gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
+		    stmt = gimple_build_assign (ref, repl);
 		  }
-		break;
+		gsi_replace (&gsi, stmt, true);
+		info.modified = true;
 	      }
+	      break;
 
 	    default:
 	      walk_gimple_op (stmt, ipa_simd_modify_stmt_ops, &wi);

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Jakub Jelinek]

On Thu, Nov 07, 2013 at 08:17:13AM -0700, Aldy Hernandez wrote:
> But as discussed on IRC, I wonder whether we can do without the
> following in the attached patch:
> 
> +		    tree repl = make_ssa_name (TREE_TYPE (retval), NULL);
> +		    stmt = gimple_build_assign (repl, retval);
> +		    gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
> +		    stmt = gimple_build_assign (ref, repl);
> 
> ...and unconditionally do:
> 
> +		  stmt = gimple_build_assign (ref, retval);

This should be sufficient.
> 
> ...since it seems all the GIMPLE_RETURNs I see can be replaced by
> ``retval_array[iter] = whatever'' without creating something
> non-gimple (thus avoiding an SSA variable).
> 
> Either way, I'm ok.  Let me know.

Thanks.

	Jakub

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Martin Jambor]

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

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Aldy Hernandez]

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

On 11/07/13 10:58, Martin Jambor wrote:

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

Done.

Note that I didn't have to move
ipa_sra_modify_function_body, since it wasn't used outside of 
tree-sra.c, and omp-low.c has its own version.  Instead I just removed 
it from ipa-prop.h where I had inadvertently placed it.

OK for branch?

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 13380 bytes --]

commit c7602b7b88a9e85c7e3076e38d471be5bc728089
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Mon Nov 11 10:10:47 2013 -0700

    	* ipa-prop.c (get_ssa_base_param): New.
    	* ipa-prop.h (ipa_modify_expr): Rename from ipa_sra_modify_expr.
    	Remove ipa_sra_modify_function_body.
    	(ipa_get_adjustment_candidate): Rename from
    	sra_ipa_get_adjustment_candidate.
    	* omp-low.c (ipa_simd_modify_stmt_ops): Rename
    	sra_ipa_get_adjustment_candidate to ipa_get_adjustment_candidate.
    	* tree-sra.c (get_ssa_base_param): Remove default_def argument.
    	(create_access): Remove lass argument to get_ssa_base_param.
    	(disqualify_base_of_expr): Same.
    	(sra_ipa_get_adjustment_candidate): Rename to
    	ipa_get_adjustment_candidate and move to ipa-prop.c.
    	(sra_ipa_modify_expr): Rename to ipa_modify_expr and move to
    	ipa-prop.c.
    	(sra_ipa_modify_assign): Rename sra_ipa_modify_expr to
    	ipa_modify_expr.
    	(ipa_sra_modify_function_body): Same.

diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 76e9b61..042d623 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -3756,6 +3756,124 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, gimple stmt,
   free_dominance_info (CDI_DOMINATORS);
 }
 
+/* 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
+ipa_modify_expr (tree *expr, bool convert,
+		 ipa_parm_adjustment_vec adjustments)
+{
+  struct ipa_parm_adjustment *cand
+    = ipa_get_adjustment_candidate (expr, &convert, adjustments, false);
+  if (!cand)
+    return false;
+
+  tree src;
+  if (cand->by_ref)
+    src = build_simple_mem_ref (cand->new_decl);
+  else
+    src = cand->new_decl;
+
+  if (dump_file && (dump_flags & TDF_DETAILS))
+    {
+      fprintf (dump_file, "About to replace expr ");
+      print_generic_expr (dump_file, *expr, 0);
+      fprintf (dump_file, " with ");
+      print_generic_expr (dump_file, src, 0);
+      fprintf (dump_file, "\n");
+    }
+
+  if (convert && !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
+    {
+      tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
+      *expr = vce;
+    }
+  else
+    *expr = src;
+  return true;
+}
+
+/* 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, bool ignore_default_def)
+{
+  if (TREE_CODE (t) == SSA_NAME)
+    {
+      if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
+	return SSA_NAME_VAR (t);
+      else
+	return NULL_TREE;
+    }
+  return t;
+}
+
+/* Given an expression, return an adjustment entry specifying the
+   transformation to be done on EXPR.  If no suitable adjustment entry
+   was found, returns NULL.
+
+   If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
+   default def, otherwise bail on them.
+
+   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 *
+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);
+      if (convert)
+	*convert = true;
+    }
+
+  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 NULL;
+
+  if (TREE_CODE (base) == MEM_REF)
+    {
+      offset += mem_ref_offset (base).low * BITS_PER_UNIT;
+      base = TREE_OPERAND (base, 0);
+    }
+
+  base = get_ssa_base_param (base, ignore_default_def);
+  if (!base || TREE_CODE (base) != PARM_DECL)
+    return NULL;
+
+  struct ipa_parm_adjustment *cand = NULL;
+  unsigned int len = adjustments.length ();
+  for (unsigned i = 0; i < len; i++)
+    {
+      struct ipa_parm_adjustment *adj = &adjustments[i];
+
+      if (adj->base == base
+	  && (adj->offset == offset || adj->op == IPA_PARM_OP_REMOVE))
+	{
+	  cand = adj;
+	  break;
+	}
+    }
+
+  if (!cand || cand->op == IPA_PARM_OP_COPY || cand->op == IPA_PARM_OP_REMOVE)
+    return NULL;
+  return cand;
+}
+
 /* Return true iff BASE_INDEX is in ADJUSTMENTS more than once.  */
 
 static bool
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index e1e8622..28ef013 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -716,15 +716,14 @@ tree ipa_value_from_jfunc (struct ipa_node_params *info,
 			   struct ipa_jump_func *jfunc);
 unsigned int ipcp_transform_function (struct cgraph_node *node);
 void ipa_dump_param (FILE *, struct ipa_node_params *info, int i);
+bool ipa_modify_expr (tree *, bool, ipa_parm_adjustment_vec);
+ipa_parm_adjustment *ipa_get_adjustment_candidate (tree *&, bool *,
+						   ipa_parm_adjustment_vec,
+						   bool);
 
 
 /* From tree-sra.c:  */
 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 51cda58..980af84 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11739,7 +11739,7 @@ ipa_simd_modify_stmt_ops (tree *tp, int *walk_subtrees, void *data)
     }
   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);
+    = ipa_get_adjustment_candidate (tp, NULL, info->adjustments, true);
   if (!cand)
     return NULL_TREE;
 
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 2f19899..721751e 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -785,17 +785,15 @@ 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.  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.  */
+/* 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.  */
 
 static tree
-get_ssa_base_param (tree t, bool ignore_default_def)
+get_ssa_base_param (tree t)
 {
   if (TREE_CODE (t) == SSA_NAME)
     {
-      if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
+      if (SSA_NAME_IS_DEFAULT_DEF (t))
 	return SSA_NAME_VAR (t);
       else
 	return NULL_TREE;
@@ -876,7 +874,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), false);
+      base = get_ssa_base_param (TREE_OPERAND (base, 0));
       if (!base)
 	return NULL;
       ptr = true;
@@ -1041,7 +1039,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), false);
+    t = get_ssa_base_param (TREE_OPERAND (t, 0));
 
   if (t && DECL_P (t))
     disqualify_candidate (t, reason);
@@ -4489,106 +4487,6 @@ replace_removed_params_ssa_names (gimple stmt,
   return true;
 }
 
-/* Given an expression, return an adjustment entry specifying the
-   transformation to be done on EXPR.  If no suitable adjustment entry
-   was found, returns NULL.
-
-   If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
-   default def, otherwise bail on them.
-
-   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);
-      if (convert)
-	*convert = true;
-    }
-
-  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 NULL;
-
-  if (TREE_CODE (base) == MEM_REF)
-    {
-      offset += mem_ref_offset (base).low * BITS_PER_UNIT;
-      base = TREE_OPERAND (base, 0);
-    }
-
-  base = get_ssa_base_param (base, ignore_default_def);
-  if (!base || TREE_CODE (base) != PARM_DECL)
-    return NULL;
-
-  struct ipa_parm_adjustment *cand = NULL;
-  unsigned int len = adjustments.length ();
-  for (unsigned i = 0; i < len; i++)
-    {
-      struct ipa_parm_adjustment *adj = &adjustments[i];
-
-      if (adj->base == base
-	  && (adj->offset == offset || adj->op == IPA_PARM_OP_REMOVE))
-	{
-	  cand = adj;
-	  break;
-	}
-    }
-
-  if (!cand || cand->op == IPA_PARM_OP_COPY || cand->op == IPA_PARM_OP_REMOVE)
-    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->new_decl);
-  else
-    src = cand->new_decl;
-
-  if (dump_file && (dump_flags & TDF_DETAILS))
-    {
-      fprintf (dump_file, "About to replace expr ");
-      print_generic_expr (dump_file, *expr, 0);
-      fprintf (dump_file, " with ");
-      print_generic_expr (dump_file, src, 0);
-      fprintf (dump_file, "\n");
-    }
-
-  if (convert && !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
-    {
-      tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
-      *expr = vce;
-    }
-  else
-    *expr = src;
-  return true;
-}
-
 /* If the statement pointed to by STMT_PTR contains any expressions that need
    to replaced with a different one as noted by ADJUSTMENTS, do so.  Handle any
    potential type incompatibilities (GSI is used to accommodate conversion
@@ -4609,8 +4507,8 @@ sra_ipa_modify_assign (gimple *stmt_ptr, gimple_stmt_iterator *gsi,
   rhs_p = gimple_assign_rhs1_ptr (stmt);
   lhs_p = gimple_assign_lhs_ptr (stmt);
 
-  any = sra_ipa_modify_expr (rhs_p, false, adjustments);
-  any |= sra_ipa_modify_expr (lhs_p, false, adjustments);
+  any = ipa_modify_expr (rhs_p, false, adjustments);
+  any |= ipa_modify_expr (lhs_p, false, adjustments);
   if (any)
     {
       tree new_rhs = NULL_TREE;
@@ -4682,7 +4580,7 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
 	    case GIMPLE_RETURN:
 	      t = gimple_return_retval_ptr (stmt);
 	      if (*t != NULL_TREE)
-		modified |= sra_ipa_modify_expr (t, true, adjustments);
+		modified |= ipa_modify_expr (t, true, adjustments);
 	      break;
 
 	    case GIMPLE_ASSIGN:
@@ -4695,13 +4593,13 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
 	      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);
+		  modified |= 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);
+		  modified |= ipa_modify_expr (t, false, adjustments);
 		  modified |= replace_removed_params_ssa_names (stmt,
 								adjustments);
 		}
@@ -4711,12 +4609,12 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
 	      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);
+		  modified |= 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);
+		  modified |= ipa_modify_expr (t, false, adjustments);
 		}
 	      break;
 

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Martin Jambor]

Hi,

On Mon, Nov 11, 2013 at 10:15:24AM -0700, Aldy Hernandez wrote:
> On 11/07/13 10:58, Martin Jambor wrote:
> 
> >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.
> 
> Done.
> 
> Note that I didn't have to move
> ipa_sra_modify_function_body, since it wasn't used outside of
> tree-sra.c, and omp-low.c has its own version.  Instead I just
> removed it from ipa-prop.h where I had inadvertently placed it.
> 

Thanks.  I only have two comments about ipa_get_adjustment_candidate:

> commit c7602b7b88a9e85c7e3076e38d471be5bc728089
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Mon Nov 11 10:10:47 2013 -0700
> 
>     	* ipa-prop.c (get_ssa_base_param): New.
>     	* ipa-prop.h (ipa_modify_expr): Rename from ipa_sra_modify_expr.
>     	Remove ipa_sra_modify_function_body.
>     	(ipa_get_adjustment_candidate): Rename from
>     	sra_ipa_get_adjustment_candidate.
>     	* omp-low.c (ipa_simd_modify_stmt_ops): Rename
>     	sra_ipa_get_adjustment_candidate to ipa_get_adjustment_candidate.
>     	* tree-sra.c (get_ssa_base_param): Remove default_def argument.
>     	(create_access): Remove lass argument to get_ssa_base_param.
>     	(disqualify_base_of_expr): Same.
>     	(sra_ipa_get_adjustment_candidate): Rename to
>     	ipa_get_adjustment_candidate and move to ipa-prop.c.
>     	(sra_ipa_modify_expr): Rename to ipa_modify_expr and move to
>     	ipa-prop.c.
>     	(sra_ipa_modify_assign): Rename sra_ipa_modify_expr to
>     	ipa_modify_expr.
>     	(ipa_sra_modify_function_body): Same.
> 
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 76e9b61..042d623 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -3756,6 +3756,124 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, gimple stmt,
>    free_dominance_info (CDI_DOMINATORS);
>  }
>  
> +/* 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
> +ipa_modify_expr (tree *expr, bool convert,
> +		 ipa_parm_adjustment_vec adjustments)
> +{
> +  struct ipa_parm_adjustment *cand
> +    = ipa_get_adjustment_candidate (expr, &convert, adjustments, false);
> +  if (!cand)
> +    return false;
> +
> +  tree src;
> +  if (cand->by_ref)
> +    src = build_simple_mem_ref (cand->new_decl);
> +  else
> +    src = cand->new_decl;
> +
> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +    {
> +      fprintf (dump_file, "About to replace expr ");
> +      print_generic_expr (dump_file, *expr, 0);
> +      fprintf (dump_file, " with ");
> +      print_generic_expr (dump_file, src, 0);
> +      fprintf (dump_file, "\n");
> +    }
> +
> +  if (convert && !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
> +    {
> +      tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
> +      *expr = vce;
> +    }
> +  else
> +    *expr = src;
> +  return true;
> +}
> +
> +/* 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, bool ignore_default_def)
> +{
> +  if (TREE_CODE (t) == SSA_NAME)
> +    {
> +      if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
> +	return SSA_NAME_VAR (t);
> +      else
> +	return NULL_TREE;
> +    }
> +  return t;
> +}
> +
> +/* Given an expression, return an adjustment entry specifying the
> +   transformation to be done on EXPR.  If no suitable adjustment entry
> +   was found, returns NULL.
> +
> +   If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
> +   default def, otherwise bail on them.
> +
> +   If CONVERT is non-NULL,

This is not true, convert can be set even when the function might
return NULL afterwards.

> + 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 *
> +ipa_get_adjustment_candidate (tree *&expr, bool *convert,
> +			      ipa_parm_adjustment_vec adjustments,
> +			      bool ignore_default_def)

I find changing parameters passed by reference confusing and
error-prone, I would very much prefer if this was "tree **expr".
Either way, you should document that expr can be changed in the
function comment.

The patch is OK with me but please at least fix the comments.

Thanks,

Martin


> +{
> +  if (TREE_CODE (*expr) == BIT_FIELD_REF
> +      || TREE_CODE (*expr) == IMAGPART_EXPR
> +      || TREE_CODE (*expr) == REALPART_EXPR)
> +    {
> +      expr = &TREE_OPERAND (*expr, 0);
> +      if (convert)
> +	*convert = true;
> +    }
> +
> +  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 NULL;
> +
> +  if (TREE_CODE (base) == MEM_REF)
> +    {
> +      offset += mem_ref_offset (base).low * BITS_PER_UNIT;
> +      base = TREE_OPERAND (base, 0);
> +    }
> +
> +  base = get_ssa_base_param (base, ignore_default_def);
> +  if (!base || TREE_CODE (base) != PARM_DECL)
> +    return NULL;
> +
> +  struct ipa_parm_adjustment *cand = NULL;
> +  unsigned int len = adjustments.length ();
> +  for (unsigned i = 0; i < len; i++)
> +    {
> +      struct ipa_parm_adjustment *adj = &adjustments[i];
> +
> +      if (adj->base == base
> +	  && (adj->offset == offset || adj->op == IPA_PARM_OP_REMOVE))
> +	{
> +	  cand = adj;
> +	  break;
> +	}
> +    }
> +
> +  if (!cand || cand->op == IPA_PARM_OP_COPY || cand->op == IPA_PARM_OP_REMOVE)
> +    return NULL;
> +  return cand;
> +}
> +
>  /* Return true iff BASE_INDEX is in ADJUSTMENTS more than once.  */
>  
>  static bool

Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Aldy Hernandez]

On 11/12/13 06:56, Martin Jambor wrote:

>> +
>> +/* Given an expression, return an adjustment entry specifying the
>> +   transformation to be done on EXPR.  If no suitable adjustment entry
>> +   was found, returns NULL.
>> +
>> +   If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
>> +   default def, otherwise bail on them.
>> +
>> +   If CONVERT is non-NULL,
>
> This is not true, convert can be set even when the function might
> return NULL afterwards.

Fixed comment.

>
>> + 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 *
>> +ipa_get_adjustment_candidate (tree *&expr, bool *convert,
>> +			      ipa_parm_adjustment_vec adjustments,
>> +			      bool ignore_default_def)
>
> I find changing parameters passed by reference confusing and
> error-prone, I would very much prefer if this was "tree **expr".
> Either way, you should document that expr can be changed in the
> function comment.

I absolutely agree.  That was sloppy on my part.  Fixed.

>
> The patch is OK with me but please at least fix the comments.

Committed to branch.

Thanks.