Patchwork Fix MINUS_EXPR vect reduction handling (PR tree-optimization/54877)

login
register
mail settings
Submitter Jakub Jelinek
Date Oct. 10, 2012, 6:42 a.m.
Message ID <20121010064218.GL26735@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/190567/
State New
Headers show

Comments

Jakub Jelinek - Oct. 10, 2012, 6:42 a.m.
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?

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.


	Jakub
Richard Guenther - Oct. 10, 2012, 8:21 a.m.
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
> 
>
Jakub Jelinek - Oct. 10, 2012, 8:39 a.m.
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
Richard Guenther - Oct. 10, 2012, 8:42 a.m.
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.
Jakub Jelinek - Oct. 10, 2012, 8:54 a.m.
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

Patch

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