[PATCH] Fix PR56321

[PATCH] Fix PR56321

[Richard Biener]

This fixes PR56321 - the unlink_stmt_vdef was a no-op as it was
called after release_defs.  Oops.

Committed as obvoious.

Richard.

2013-02-18  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/56321
	* tree-ssa-reassoc.c (propagate_op_to_single_use): Properly
	order SSA name release and virtual operand unlinking.

	* gcc.dg/torture/pr56321.c: New testcase.

Index: gcc/tree-ssa-reassoc.c
===================================================================
*** gcc/tree-ssa-reassoc.c	(revision 196115)
--- gcc/tree-ssa-reassoc.c	(working copy)
*************** propagate_op_to_single_use (tree op, gim
*** 1062,1072 ****
    if (TREE_CODE (op) != SSA_NAME)
      update_stmt (use_stmt);
    gsi = gsi_for_stmt (stmt);
    gsi_remove (&gsi, true);
    release_defs (stmt);
- 
-   if (is_gimple_call (stmt))
-     unlink_stmt_vdef (stmt);
  }
  
  /* Walks the linear chain with result *DEF searching for an operation
--- 1062,1070 ----
    if (TREE_CODE (op) != SSA_NAME)
      update_stmt (use_stmt);
    gsi = gsi_for_stmt (stmt);
+   unlink_stmt_vdef (stmt);
    gsi_remove (&gsi, true);
    release_defs (stmt);
  }
  
  /* Walks the linear chain with result *DEF searching for an operation
Index: gcc/testsuite/gcc.dg/torture/pr56321.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr56321.c	(revision 0)
--- gcc/testsuite/gcc.dg/torture/pr56321.c	(working copy)
***************
*** 0 ****
--- 1,11 ----
+ /* { dg-do compile } */
+ /* { dg-options "-ffast-math" } */
+ 
+ void foo(int n, int nreps, float tdgefa, float tdgesl)
+ {
+   float kflops,ops;
+   ops=((2.0*n*n*n)/3.0+2.0*n*n);
+   kflops=2.*nreps*ops/(1000.*(tdgefa+tdgesl));
+ 
+   __builtin_printf ("%f\n", kflops);
+ }

Re: [PATCH] Fix PR56321

[Bill Schmidt]

On Mon, 2013-02-18 at 13:05 +0100, Richard Biener wrote:
> On Fri, Feb 15, 2013 at 6:53 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
> > When we remove __builtin_pow statements as part of reassociation, we
> > have to unlink the associated VDEF.  We've always done this when we
> > directly remove the statement.  However, in reassociation the statements
> > are sometimes modified in place instead of removed, potentially leaving
> > one or more dangling VUSEs.  This patch solves the problem by unlinking
> > the VDEF when the statement's operands are added to the ops list.
> >
> > Bootstrapped and regression tested on powerpc64-unknown-linux-gnu with
> > no new regressions.  The new test case is the code that exposed the
> > problem in PR56321.  Ok for trunk?
> 
> No, that's way to complicated.  The issue is that
> 
> static void
> propagate_op_to_single_use (tree op, gimple stmt, tree *def)
> {
> ...
>   gsi = gsi_for_stmt (stmt);
>   gsi_remove (&gsi, true);
>   release_defs (stmt);
> 
>   if (is_gimple_call (stmt))
>     unlink_stmt_vdef (stmt);
> 
> tries to unlink the stmts VDEF after releasing it.  That doesn't work.
> 
> A proper fix is
> 
> Index: tree-ssa-reassoc.c
> ===================================================================
> --- tree-ssa-reassoc.c  (revision 196115)
> +++ tree-ssa-reassoc.c  (working copy)
> @@ -1062,11 +1062,9 @@ propagate_op_to_single_use (tree op, gim
>    if (TREE_CODE (op) != SSA_NAME)
>      update_stmt (use_stmt);
>    gsi = gsi_for_stmt (stmt);
> +  unlink_stmt_vdef (stmt);
>    gsi_remove (&gsi, true);
>    release_defs (stmt);
> -
> -  if (is_gimple_call (stmt))
> -    unlink_stmt_vdef (stmt);
>  }
> 
>  /* Walks the linear chain with result *DEF searching for an operation
> 
> I'll take care of it in a second.

OK, thanks!  Appreciate it.

Bill

> 
> Thanks,
> Richard.
> 
> 
> > Thanks,
> > Bill
> >
> >
> > Index: gcc/testsuite/gcc.dg/tree-ssa/pr56321.c
> > ===================================================================
> > --- gcc/testsuite/gcc.dg/tree-ssa/pr56321.c     (revision 0)
> > +++ gcc/testsuite/gcc.dg/tree-ssa/pr56321.c     (revision 0)
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O3 -ffast-math -fdump-tree-optimized" } */
> > +
> > +float foo(int n)
> > +{
> > +  return ((2.0*n*n)/3.0+2.0*n);
> > +}
> > +
> > +/* { dg-final { scan-tree-dump-times "__builtin_pow" 0 "optimized" } } */
> > +/* { dg-final { scan-tree-dump-times " \\* " 2 "optimized" } } */
> > +/* { dg-final { scan-tree-dump-times " \\+ " 1 "optimized" } } */
> > +/* { dg-final { cleanup-tree-dump "optimized" } } */
> > Index: gcc/tree-ssa-reassoc.c
> > ===================================================================
> > --- gcc/tree-ssa-reassoc.c      (revision 196053)
> > +++ gcc/tree-ssa-reassoc.c      (working copy)
> > @@ -3386,6 +3386,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
> >             {
> >               add_repeat_to_ops_vec (ops, base, exponent);
> >               gimple_set_visited (binrhsdef, true);
> > +             // We may not physically remove the call later because
> > +             // stmts are preferably modified in place.  But we have
> > +             // to remove any VDEF associated with the call regardless.
> > +             unlink_stmt_vdef (binrhsdef);
> >             }
> >           else
> >             add_to_ops_vec (ops, binrhs);
> > @@ -3396,6 +3400,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
> >             {
> >               add_repeat_to_ops_vec (ops, base, exponent);
> >               gimple_set_visited (binlhsdef, true);
> > +             // We may not physically remove the call later because
> > +             // stmts are preferably modified in place.  But we have
> > +             // to remove any VDEF associated with the call regardless.
> > +             unlink_stmt_vdef (binlhsdef);
> >             }
> >           else
> >             add_to_ops_vec (ops, binlhs);
> > @@ -3445,6 +3453,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
> >      {
> >        add_repeat_to_ops_vec (ops, base, exponent);
> >        gimple_set_visited (SSA_NAME_DEF_STMT (binrhs), true);
> > +      // We may not physically remove the call later because
> > +      // stmts are preferably modified in place.  But we have
> > +      // to remove any VDEF associated with the call regardless.
> > +      unlink_stmt_vdef (SSA_NAME_DEF_STMT (binrhs));
> >      }
> >    else
> >      add_to_ops_vec (ops, binrhs);
> >
> >
> 

Re: [PATCH] Fix PR56321

[Richard Biener]

On Fri, Feb 15, 2013 at 6:53 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> When we remove __builtin_pow statements as part of reassociation, we
> have to unlink the associated VDEF.  We've always done this when we
> directly remove the statement.  However, in reassociation the statements
> are sometimes modified in place instead of removed, potentially leaving
> one or more dangling VUSEs.  This patch solves the problem by unlinking
> the VDEF when the statement's operands are added to the ops list.
>
> Bootstrapped and regression tested on powerpc64-unknown-linux-gnu with
> no new regressions.  The new test case is the code that exposed the
> problem in PR56321.  Ok for trunk?

No, that's way to complicated.  The issue is that

static void
propagate_op_to_single_use (tree op, gimple stmt, tree *def)
{
...
  gsi = gsi_for_stmt (stmt);
  gsi_remove (&gsi, true);
  release_defs (stmt);

  if (is_gimple_call (stmt))
    unlink_stmt_vdef (stmt);

tries to unlink the stmts VDEF after releasing it.  That doesn't work.

A proper fix is

Index: tree-ssa-reassoc.c
===================================================================
--- tree-ssa-reassoc.c  (revision 196115)
+++ tree-ssa-reassoc.c  (working copy)
@@ -1062,11 +1062,9 @@ propagate_op_to_single_use (tree op, gim
   if (TREE_CODE (op) != SSA_NAME)
     update_stmt (use_stmt);
   gsi = gsi_for_stmt (stmt);
+  unlink_stmt_vdef (stmt);
   gsi_remove (&gsi, true);
   release_defs (stmt);
-
-  if (is_gimple_call (stmt))
-    unlink_stmt_vdef (stmt);
 }

 /* Walks the linear chain with result *DEF searching for an operation

I'll take care of it in a second.

Thanks,
Richard.


> Thanks,
> Bill
>
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr56321.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr56321.c     (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr56321.c     (revision 0)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -ffast-math -fdump-tree-optimized" } */
> +
> +float foo(int n)
> +{
> +  return ((2.0*n*n)/3.0+2.0*n);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin_pow" 0 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times " \\* " 2 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times " \\+ " 1 "optimized" } } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: gcc/tree-ssa-reassoc.c
> ===================================================================
> --- gcc/tree-ssa-reassoc.c      (revision 196053)
> +++ gcc/tree-ssa-reassoc.c      (working copy)
> @@ -3386,6 +3386,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
>             {
>               add_repeat_to_ops_vec (ops, base, exponent);
>               gimple_set_visited (binrhsdef, true);
> +             // We may not physically remove the call later because
> +             // stmts are preferably modified in place.  But we have
> +             // to remove any VDEF associated with the call regardless.
> +             unlink_stmt_vdef (binrhsdef);
>             }
>           else
>             add_to_ops_vec (ops, binrhs);
> @@ -3396,6 +3400,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
>             {
>               add_repeat_to_ops_vec (ops, base, exponent);
>               gimple_set_visited (binlhsdef, true);
> +             // We may not physically remove the call later because
> +             // stmts are preferably modified in place.  But we have
> +             // to remove any VDEF associated with the call regardless.
> +             unlink_stmt_vdef (binlhsdef);
>             }
>           else
>             add_to_ops_vec (ops, binlhs);
> @@ -3445,6 +3453,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
>      {
>        add_repeat_to_ops_vec (ops, base, exponent);
>        gimple_set_visited (SSA_NAME_DEF_STMT (binrhs), true);
> +      // We may not physically remove the call later because
> +      // stmts are preferably modified in place.  But we have
> +      // to remove any VDEF associated with the call regardless.
> +      unlink_stmt_vdef (SSA_NAME_DEF_STMT (binrhs));
>      }
>    else
>      add_to_ops_vec (ops, binrhs);
>
>

[PATCH] Fix PR56321

[Bill Schmidt]

When we remove __builtin_pow statements as part of reassociation, we
have to unlink the associated VDEF.  We've always done this when we
directly remove the statement.  However, in reassociation the statements
are sometimes modified in place instead of removed, potentially leaving
one or more dangling VUSEs.  This patch solves the problem by unlinking
the VDEF when the statement's operands are added to the ops list.

Bootstrapped and regression tested on powerpc64-unknown-linux-gnu with
no new regressions.  The new test case is the code that exposed the
problem in PR56321.  Ok for trunk?

Thanks,
Bill


Index: gcc/testsuite/gcc.dg/tree-ssa/pr56321.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr56321.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr56321.c	(revision 0)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -ffast-math -fdump-tree-optimized" } */
+
+float foo(int n)
+{                              
+  return ((2.0*n*n)/3.0+2.0*n);   
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin_pow" 0 "optimized" } } */
+/* { dg-final { scan-tree-dump-times " \\* " 2 "optimized" } } */
+/* { dg-final { scan-tree-dump-times " \\+ " 1 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
Index: gcc/tree-ssa-reassoc.c
===================================================================
--- gcc/tree-ssa-reassoc.c	(revision 196053)
+++ gcc/tree-ssa-reassoc.c	(working copy)
@@ -3386,6 +3386,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
 	    {
 	      add_repeat_to_ops_vec (ops, base, exponent);
 	      gimple_set_visited (binrhsdef, true);
+	      // We may not physically remove the call later because
+	      // stmts are preferably modified in place.  But we have
+	      // to remove any VDEF associated with the call regardless.
+	      unlink_stmt_vdef (binrhsdef);
 	    }
 	  else
 	    add_to_ops_vec (ops, binrhs);
@@ -3396,6 +3400,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
 	    {
 	      add_repeat_to_ops_vec (ops, base, exponent);
 	      gimple_set_visited (binlhsdef, true);
+	      // We may not physically remove the call later because
+	      // stmts are preferably modified in place.  But we have
+	      // to remove any VDEF associated with the call regardless.
+	      unlink_stmt_vdef (binlhsdef);
 	    }
 	  else
 	    add_to_ops_vec (ops, binlhs);
@@ -3445,6 +3453,10 @@ linearize_expr_tree (vec<operand_entry_t> *ops, gi
     {
       add_repeat_to_ops_vec (ops, base, exponent);
       gimple_set_visited (SSA_NAME_DEF_STMT (binrhs), true);
+      // We may not physically remove the call later because
+      // stmts are preferably modified in place.  But we have
+      // to remove any VDEF associated with the call regardless.
+      unlink_stmt_vdef (SSA_NAME_DEF_STMT (binrhs));
     }
   else
     add_to_ops_vec (ops, binrhs);