| Message ID | 5277BAF0.6050506@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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.
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
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; +}
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.