* [PATCH] Fix PR56321
@ 2013-02-15 17:54 Bill Schmidt
2013-02-18 12:05 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Bill Schmidt @ 2013-02-15 17:54 UTC (permalink / raw)
To: gcc-patches; +Cc: richard.guenther, bergner
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);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Fix PR56321
2013-02-15 17:54 [PATCH] Fix PR56321 Bill Schmidt
@ 2013-02-18 12:05 ` Richard Biener
2013-02-18 13:26 ` Bill Schmidt
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2013-02-18 12:05 UTC (permalink / raw)
To: Bill Schmidt; +Cc: gcc-patches, bergner
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);
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Fix PR56321
2013-02-18 12:05 ` Richard Biener
@ 2013-02-18 13:26 ` Bill Schmidt
0 siblings, 0 replies; 4+ messages in thread
From: Bill Schmidt @ 2013-02-18 13:26 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, bergner
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);
> >
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Fix PR56321
@ 2013-02-18 12:10 Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2013-02-18 12:10 UTC (permalink / raw)
To: gcc-patches
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);
+ }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-02-18 13:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-15 17:54 [PATCH] Fix PR56321 Bill Schmidt
2013-02-18 12:05 ` Richard Biener
2013-02-18 13:26 ` Bill Schmidt
2013-02-18 12:10 Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).