[omp4]
diff mbox

Message ID 5277BAF0.6050506@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Nov. 4, 2013, 3:19 p.m. UTC
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
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.

Comments

Aldy Hernandez Nov. 4, 2013, 3:22 p.m. UTC | #1
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.
Jakub Jelinek Nov. 4, 2013, 3:44 p.m. UTC | #2
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
Aldy Hernandez Nov. 4, 2013, 4:12 p.m. UTC | #3
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
Jakub Jelinek Nov. 4, 2013, 4:16 p.m. UTC | #4
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
Aldy Hernandez Nov. 4, 2013, 4:22 p.m. UTC | #5
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.
Jakub Jelinek Nov. 4, 2013, 4:25 p.m. UTC | #6
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

Patch
diff mbox

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;
+}