| Message ID | 20121010064218.GL26735@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 10 Oct 2012, Jakub Jelinek wrote: > Hi! > > As the testcase shows, a REAL_CST can appear in MINUS_EXPR as second operand > (that is not canonicalized to PLUS_EXPR of negated value, unlike integer > constants (with exception of minimum value)). The following patch handles > constants there and punts if it isn't SSA_NAME or a constant that can be > negated. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Err, why? Can't we negate a constant, too? The ICE simply means we should have used make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) instead of copy_ssa_name. Ok with a fix that way. Thanks, Richard. > 2012-10-10 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/54877 > * tree-vect-loop.c (vect_is_simple_reduction_1): For MINUS_EXPR, > check if rhs2 is SSA_NAME, if not and is constant, try to negate > it, otherwise bail out. > > * gcc.dg/torture/pr54877.c: New test. > > --- gcc/tree-vect-loop.c.jj 2012-10-05 13:21:22.000000000 +0200 > +++ gcc/tree-vect-loop.c 2012-10-09 20:45:18.559048120 +0200 > @@ -2382,16 +2382,41 @@ vect_is_simple_reduction_1 (loop_vec_inf > if (orig_code == MINUS_EXPR) > { > tree rhs = gimple_assign_rhs2 (def_stmt); > - tree negrhs = copy_ssa_name (rhs, NULL); > - gimple negate_stmt = gimple_build_assign_with_ops (NEGATE_EXPR, negrhs, > - rhs, NULL); > - gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); > - set_vinfo_for_stmt (negate_stmt, new_stmt_vec_info (negate_stmt, > - loop_info, NULL)); > - gsi_insert_before (&gsi, negate_stmt, GSI_NEW_STMT); > - gimple_assign_set_rhs2 (def_stmt, negrhs); > - gimple_assign_set_rhs_code (def_stmt, PLUS_EXPR); > - update_stmt (def_stmt); > + > + if (TREE_CODE (rhs) == SSA_NAME) > + { > + tree negrhs = copy_ssa_name (rhs, NULL); > + gimple negate_stmt > + = gimple_build_assign_with_ops (NEGATE_EXPR, negrhs, rhs, NULL); > + gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); > + set_vinfo_for_stmt (negate_stmt, > + new_stmt_vec_info (negate_stmt, > + loop_info, NULL)); > + gsi_insert_before (&gsi, negate_stmt, GSI_NEW_STMT); > + gimple_assign_set_rhs2 (def_stmt, negrhs); > + gimple_assign_set_rhs_code (def_stmt, PLUS_EXPR); > + update_stmt (def_stmt); > + orig_code = PLUS_EXPR; > + } > + else if (is_gimple_constant (rhs) > + && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (rhs)))) > + { > + tree negrhs = fold_unary (NEGATE_EXPR, TREE_TYPE (rhs), rhs); > + if (negrhs && is_gimple_val (negrhs)) > + { > + gimple_assign_set_rhs2 (def_stmt, negrhs); > + gimple_assign_set_rhs_code (def_stmt, PLUS_EXPR); > + update_stmt (def_stmt); > + orig_code = PLUS_EXPR; > + } > + } > + if (orig_code == MINUS_EXPR) > + { > + if (dump_kind_p (MSG_NOTE)) > + report_vect_op (MSG_NOTE, def_stmt, > + "reduction: unhandled minus expr: "); > + return NULL; > + } > } > > /* Reduction is safe. We're dealing with one of the following: > --- gcc/testsuite/gcc.dg/torture/pr54877.c.jj 2012-10-09 20:50:43.083259039 +0200 > +++ gcc/testsuite/gcc.dg/torture/pr54877.c 2012-10-09 20:51:40.463941703 +0200 > @@ -0,0 +1,23 @@ > +/* PR tree-optimization/54877 */ > +/* { dg-do run } */ > +/* { dg-options "-ffast-math" } */ > + > +extern void abort (void); > + > +int > +foo (void) > +{ > + double d; > + int i; > + for (i = 0, d = 0; i < 64; i++) > + d--; > + return (int) d; > +} > + > +int > +main () > +{ > + if (foo () != -64) > + abort (); > + return 0; > +} > > Jakub > >
On Wed, Oct 10, 2012 at 10:21:59AM +0200, Richard Biener wrote: > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Err, why? Can't we negate a constant, too? Not all of them, say INT_MIN can't be negated. Though, int foo (int n) { int i, d; for (i = 0, d = 0; i < n; i++) d -= -__INT_MAX__ - 1; return d; } doesn't trigger it. And for constants I thought it would be better to just fold it immediately. > The ICE simply means > we should have used make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) > instead of copy_ssa_name. Is make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) preferrable over just make_ssa_name (TREE_TYPE (rhs), NULL); ? Jakub
On Wed, 10 Oct 2012, Jakub Jelinek wrote: > On Wed, Oct 10, 2012 at 10:21:59AM +0200, Richard Biener wrote: > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > Err, why? Can't we negate a constant, too? > > Not all of them, say INT_MIN can't be negated. > Though, > int > foo (int n) > { > int i, d; > for (i = 0, d = 0; i < n; i++) > d -= -__INT_MAX__ - 1; > return d; > } > doesn't trigger it. And for constants I thought it would be better to > just fold it immediately. Yeah, I meant we can negate all constants by doing tem = - CONSTANT; right? Or wait ... even if we have red = x - y; if y is INT_MIN then the code we just created may have introduced undefined overflow (of course you could argue that with x - y it is very likely that undefined overflow already happened) ... > > The ICE simply means > > we should have used make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) > > instead of copy_ssa_name. > > Is make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) preferrable over just > make_ssa_name (TREE_TYPE (rhs), NULL); ? Ah, no, make_ssa_name (TREE_TYPE (rhs), NULL) is even better. On the 4.7 branch you need to create a new temp var ... Richard.
On Wed, Oct 10, 2012 at 10:42:10AM +0200, Richard Biener wrote: > Yeah, I meant we can negate all constants by doing > > tem = - CONSTANT; Yeah, that is what I meant. tem = - INT_MIN; is undefined overflow. > > right? Or wait ... even if we have > > red = x - y; > > if y is INT_MIN then the code we just created may have introduced > undefined overflow (of course you could argue that with x - y it > is very likely that undefined overflow already happened) ... Not for negative x. x = -3; y = -__INT_MAX__ - 1; red = x - y; is IMHO fine. > > > The ICE simply means > > > we should have used make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) > > > instead of copy_ssa_name. > > > > Is make_temp_ssa_name (TREE_TYPE (rhs), NULL, NULL) preferrable over just > > make_ssa_name (TREE_TYPE (rhs), NULL); ? > > Ah, no, make_ssa_name (TREE_TYPE (rhs), NULL) is even better. On > the 4.7 branch you need to create a new temp var ... Sure. Jakub
--- gcc/tree-vect-loop.c.jj 2012-10-05 13:21:22.000000000 +0200 +++ gcc/tree-vect-loop.c 2012-10-09 20:45:18.559048120 +0200 @@ -2382,16 +2382,41 @@ vect_is_simple_reduction_1 (loop_vec_inf if (orig_code == MINUS_EXPR) { tree rhs = gimple_assign_rhs2 (def_stmt); - tree negrhs = copy_ssa_name (rhs, NULL); - gimple negate_stmt = gimple_build_assign_with_ops (NEGATE_EXPR, negrhs, - rhs, NULL); - gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); - set_vinfo_for_stmt (negate_stmt, new_stmt_vec_info (negate_stmt, - loop_info, NULL)); - gsi_insert_before (&gsi, negate_stmt, GSI_NEW_STMT); - gimple_assign_set_rhs2 (def_stmt, negrhs); - gimple_assign_set_rhs_code (def_stmt, PLUS_EXPR); - update_stmt (def_stmt); + + if (TREE_CODE (rhs) == SSA_NAME) + { + tree negrhs = copy_ssa_name (rhs, NULL); + gimple negate_stmt + = gimple_build_assign_with_ops (NEGATE_EXPR, negrhs, rhs, NULL); + gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); + set_vinfo_for_stmt (negate_stmt, + new_stmt_vec_info (negate_stmt, + loop_info, NULL)); + gsi_insert_before (&gsi, negate_stmt, GSI_NEW_STMT); + gimple_assign_set_rhs2 (def_stmt, negrhs); + gimple_assign_set_rhs_code (def_stmt, PLUS_EXPR); + update_stmt (def_stmt); + orig_code = PLUS_EXPR; + } + else if (is_gimple_constant (rhs) + && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (rhs)))) + { + tree negrhs = fold_unary (NEGATE_EXPR, TREE_TYPE (rhs), rhs); + if (negrhs && is_gimple_val (negrhs)) + { + gimple_assign_set_rhs2 (def_stmt, negrhs); + gimple_assign_set_rhs_code (def_stmt, PLUS_EXPR); + update_stmt (def_stmt); + orig_code = PLUS_EXPR; + } + } + if (orig_code == MINUS_EXPR) + { + if (dump_kind_p (MSG_NOTE)) + report_vect_op (MSG_NOTE, def_stmt, + "reduction: unhandled minus expr: "); + return NULL; + } } /* Reduction is safe. We're dealing with one of the following: --- gcc/testsuite/gcc.dg/torture/pr54877.c.jj 2012-10-09 20:50:43.083259039 +0200 +++ gcc/testsuite/gcc.dg/torture/pr54877.c 2012-10-09 20:51:40.463941703 +0200 @@ -0,0 +1,23 @@ +/* PR tree-optimization/54877 */ +/* { dg-do run } */ +/* { dg-options "-ffast-math" } */ + +extern void abort (void); + +int +foo (void) +{ + double d; + int i; + for (i = 0, d = 0; i < 64; i++) + d--; + return (int) d; +} + +int +main () +{ + if (foo () != -64) + abort (); + return 0; +}