* [PATCH] Fix wrong-code aggregate propagate_with_phi bug (PR tree-optimization/81365)
@ 2017-07-13 20:39 Jakub Jelinek
2017-07-17 8:47 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2017-07-13 20:39 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
As mentioned in the PR, for aggregate copies we fail to verify there
are no loads in between the PHI and the aggregate copy that could alias with the
lhs of the copy, which is needed, because we want to hoist the aggregate
copy onto predecessor edges of the bb with the PHI.
The following patch implements that, bootstrapped/regtested on x86_64-linux
and i686-linux, ok for trunk and after a while 7.x?
2017-07-13 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/81365
* tree-ssa-phiprop.c (propagate_with_phi): When considering hoisting
aggregate moves onto bb predecessor edges, make sure there are no
loads that could alias the lhs in between the start of bb and the
loads from *phi. If there are any debug stmts that could alias,
reset them.
* g++.dg/torture/pr81365.C: New test.
--- gcc/tree-ssa-phiprop.c.jj 2017-05-22 10:50:11.000000000 +0200
+++ gcc/tree-ssa-phiprop.c 2017-07-11 16:52:41.012340615 +0200
@@ -327,7 +327,7 @@ propagate_with_phi (basic_block bb, gphi
if (!dominated_by_p (CDI_POST_DOMINATORS,
bb, gimple_bb (use_stmt)))
continue;
-
+
/* Check whether this is a load of *ptr. */
if (!(is_gimple_assign (use_stmt)
&& gimple_assign_rhs_code (use_stmt) == MEM_REF
@@ -356,6 +356,9 @@ propagate_with_phi (basic_block bb, gphi
insert aggregate copies on the edges instead. */
if (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
{
+ if (!gimple_vdef (use_stmt))
+ goto next;
+
/* As we replicate the lhs on each incoming edge all
used SSA names have to be available there. */
if (! for_each_index (gimple_assign_lhs_ptr (use_stmt),
@@ -363,6 +366,51 @@ propagate_with_phi (basic_block bb, gphi
get_immediate_dominator (CDI_DOMINATORS,
gimple_bb (phi))))
goto next;
+
+ gimple *vuse_stmt;
+ imm_use_iterator vui;
+ use_operand_p vuse_p;
+ bool debug_use_seen = false;
+ /* In order to move the aggregate copies earlier, make sure
+ there are no statements that could read from memory
+ aliasing the lhs in between the start of bb and use_stmt.
+ As we require use_stmt to have a VDEF above, loads after
+ use_stmt will use a different virtual SSA_NAME. */
+ FOR_EACH_IMM_USE_FAST (vuse_p, vui, vuse)
+ {
+ vuse_stmt = USE_STMT (vuse_p);
+ if (vuse_stmt == use_stmt)
+ continue;
+ if (!dominated_by_p (CDI_DOMINATORS,
+ gimple_bb (vuse_stmt), bb))
+ continue;
+ if (ref_maybe_used_by_stmt_p (vuse_stmt,
+ gimple_assign_lhs (use_stmt)))
+ {
+ if (is_gimple_debug (vuse_stmt))
+ debug_use_seen = true;
+ else
+ goto next;
+ }
+ }
+ /* Debug stmt uses should not prevent the transformation, but
+ if we saw any, reset those debug stmts. */
+ if (debug_use_seen)
+ FOR_EACH_IMM_USE_STMT (vuse_stmt, vui, vuse)
+ {
+ if (!is_gimple_debug (vuse_stmt))
+ continue;
+ if (!dominated_by_p (CDI_DOMINATORS,
+ gimple_bb (vuse_stmt), bb))
+ continue;
+ if (ref_maybe_used_by_stmt_p (vuse_stmt,
+ gimple_assign_lhs (use_stmt)))
+ {
+ gimple_debug_bind_reset_value (vuse_stmt);
+ update_stmt (vuse_stmt);
+ }
+ }
+
phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
/* Remove old stmt. The phi is taken care of by DCE. */
--- gcc/testsuite/g++.dg/torture/pr81365.C.jj 2017-07-11 17:07:11.107130111 +0200
+++ gcc/testsuite/g++.dg/torture/pr81365.C 2017-07-11 17:06:52.000000000 +0200
@@ -0,0 +1,39 @@
+// PR tree-optimization/81365
+// { dg-do run }
+
+struct A { unsigned a; };
+
+struct B {
+ B (const A *x)
+ {
+ __builtin_memcpy (b, x, 3 * sizeof (A));
+ __builtin_memcpy (c, x + 3, sizeof (A));
+ __builtin_memset (c + 1, 0, sizeof (A));
+ }
+ bool
+ foo (unsigned x)
+ {
+ A *it = c;
+ if (it->a == x || (++it)->a == x)
+ {
+ A t(b[0]);
+ b[0] = *it;
+ *it = t;
+ return true;
+ }
+ return false;
+ }
+ A b[3];
+ A c[2];
+};
+
+int
+main ()
+{
+ A x[] = { 4, 8, 12, 18 };
+ B y(x);
+ if (!y.foo (18))
+ __builtin_abort ();
+ if (!y.foo (4))
+ __builtin_abort ();
+}
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] Fix wrong-code aggregate propagate_with_phi bug (PR tree-optimization/81365)
2017-07-13 20:39 [PATCH] Fix wrong-code aggregate propagate_with_phi bug (PR tree-optimization/81365) Jakub Jelinek
@ 2017-07-17 8:47 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2017-07-17 8:47 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Thu, 13 Jul 2017, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, for aggregate copies we fail to verify there
> are no loads in between the PHI and the aggregate copy that could alias with the
> lhs of the copy, which is needed, because we want to hoist the aggregate
> copy onto predecessor edges of the bb with the PHI.
>
> The following patch implements that, bootstrapped/regtested on x86_64-linux
> and i686-linux, ok for trunk and after a while 7.x?
>
> 2017-07-13 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/81365
> * tree-ssa-phiprop.c (propagate_with_phi): When considering hoisting
> aggregate moves onto bb predecessor edges, make sure there are no
> loads that could alias the lhs in between the start of bb and the
> loads from *phi. If there are any debug stmts that could alias,
> reset them.
>
> * g++.dg/torture/pr81365.C: New test.
>
> --- gcc/tree-ssa-phiprop.c.jj 2017-05-22 10:50:11.000000000 +0200
> +++ gcc/tree-ssa-phiprop.c 2017-07-11 16:52:41.012340615 +0200
> @@ -327,7 +327,7 @@ propagate_with_phi (basic_block bb, gphi
> if (!dominated_by_p (CDI_POST_DOMINATORS,
> bb, gimple_bb (use_stmt)))
> continue;
> -
> +
> /* Check whether this is a load of *ptr. */
> if (!(is_gimple_assign (use_stmt)
> && gimple_assign_rhs_code (use_stmt) == MEM_REF
> @@ -356,6 +356,9 @@ propagate_with_phi (basic_block bb, gphi
> insert aggregate copies on the edges instead. */
> if (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
> {
> + if (!gimple_vdef (use_stmt))
> + goto next;
> +
> /* As we replicate the lhs on each incoming edge all
> used SSA names have to be available there. */
> if (! for_each_index (gimple_assign_lhs_ptr (use_stmt),
> @@ -363,6 +366,51 @@ propagate_with_phi (basic_block bb, gphi
> get_immediate_dominator (CDI_DOMINATORS,
> gimple_bb (phi))))
> goto next;
> +
> + gimple *vuse_stmt;
> + imm_use_iterator vui;
> + use_operand_p vuse_p;
> + bool debug_use_seen = false;
> + /* In order to move the aggregate copies earlier, make sure
> + there are no statements that could read from memory
> + aliasing the lhs in between the start of bb and use_stmt.
> + As we require use_stmt to have a VDEF above, loads after
> + use_stmt will use a different virtual SSA_NAME. */
> + FOR_EACH_IMM_USE_FAST (vuse_p, vui, vuse)
> + {
> + vuse_stmt = USE_STMT (vuse_p);
> + if (vuse_stmt == use_stmt)
> + continue;
> + if (!dominated_by_p (CDI_DOMINATORS,
> + gimple_bb (vuse_stmt), bb))
> + continue;
> + if (ref_maybe_used_by_stmt_p (vuse_stmt,
> + gimple_assign_lhs (use_stmt)))
> + {
> + if (is_gimple_debug (vuse_stmt))
debug stmts do not have virtual operands so their handling is not
necessary here.
Ok with that change.
Thanks,
Richard.
> + debug_use_seen = true;
> + else
> + goto next;
> + }
> + }
> + /* Debug stmt uses should not prevent the transformation, but
> + if we saw any, reset those debug stmts. */
> + if (debug_use_seen)
> + FOR_EACH_IMM_USE_STMT (vuse_stmt, vui, vuse)
> + {
> + if (!is_gimple_debug (vuse_stmt))
> + continue;
> + if (!dominated_by_p (CDI_DOMINATORS,
> + gimple_bb (vuse_stmt), bb))
> + continue;
> + if (ref_maybe_used_by_stmt_p (vuse_stmt,
> + gimple_assign_lhs (use_stmt)))
> + {
> + gimple_debug_bind_reset_value (vuse_stmt);
> + update_stmt (vuse_stmt);
> + }
> + }
> +
> phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
>
> /* Remove old stmt. The phi is taken care of by DCE. */
> --- gcc/testsuite/g++.dg/torture/pr81365.C.jj 2017-07-11 17:07:11.107130111 +0200
> +++ gcc/testsuite/g++.dg/torture/pr81365.C 2017-07-11 17:06:52.000000000 +0200
> @@ -0,0 +1,39 @@
> +// PR tree-optimization/81365
> +// { dg-do run }
> +
> +struct A { unsigned a; };
> +
> +struct B {
> + B (const A *x)
> + {
> + __builtin_memcpy (b, x, 3 * sizeof (A));
> + __builtin_memcpy (c, x + 3, sizeof (A));
> + __builtin_memset (c + 1, 0, sizeof (A));
> + }
> + bool
> + foo (unsigned x)
> + {
> + A *it = c;
> + if (it->a == x || (++it)->a == x)
> + {
> + A t(b[0]);
> + b[0] = *it;
> + *it = t;
> + return true;
> + }
> + return false;
> + }
> + A b[3];
> + A c[2];
> +};
> +
> +int
> +main ()
> +{
> + A x[] = { 4, 8, 12, 18 };
> + B y(x);
> + if (!y.foo (18))
> + __builtin_abort ();
> + if (!y.foo (4))
> + __builtin_abort ();
> +}
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-07-17 8:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-13 20:39 [PATCH] Fix wrong-code aggregate propagate_with_phi bug (PR tree-optimization/81365) Jakub Jelinek
2017-07-17 8:47 ` 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).