------- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-31
08:41 -------
Subject: Re: [PR tree-optimization/20640] add phi args to dests of
dce-redirected edges
On Mar 30, 2005, Jeffrey A Law <[EMAIL PROTECTED]> wrote:
> - PENDING_STMT (EDGE_SUCC (bb, 0)) = NULL;
> + flush_pending_stmts (EDGE_SUCC (bb, 0));
> I'm having trouble seeing how this can be correct.
After looking at what flush_pending_stmts() actually does, I must
confess I'm disappointed. I expected it to be far smarter than it
actually is.
Here's a newer version of the patch that survived bootstrap on
x86_64-linux-gnu, with default BOOT_CFLAGS in mainline, and with
BOOT_CFLAGS='-O3 -g -funroll-all-loops' in the 4.0 branch. I found
that the 4.0 branch would fail to bootstrap even with default
BOOT_CFLAGS if I added code to flush_pending_stmts() to verify that
the PHI args actually matched the corresponding PHI nodes.
My concern is that, if the code in this patch fails and we reach the
hopefully-unreachable point, we won't be able to obtain a PHI arg very
easily. I have a gut feeling that we'll always get a PHI arg from one
of the previous successors of the src of the redirected edge, but I
can't quite prove it. What do you think?
> This code is triggered rarely, I would expect it to be even rarer still
> for POST_DOM_BB to have PHI nodes. You could probably just ignore dead
> control statements where the post dominator has PHI nodes and I doubt
> it would ever be noticed.
Index: gcc/ChangeLog
from Alexandre Oliva <[EMAIL PROTECTED]>
PR tree-optimization/20640
* tree-ssa-dce.c (remove_dead_stmt): Add phi args to phi nodes
affected by an edge redirection.
Index: gcc/tree-ssa-dce.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-ssa-dce.c,v
retrieving revision 2.37
diff -u -p -r2.37 tree-ssa-dce.c
--- gcc/tree-ssa-dce.c 12 Mar 2005 20:53:18 -0000 2.37
+++ gcc/tree-ssa-dce.c 31 Mar 2005 06:39:48 -0000
@@ -724,6 +724,10 @@ remove_dead_stmt (block_stmt_iterator *i
if (is_ctrl_stmt (t))
{
basic_block post_dom_bb;
+ edge e;
+ tree phi;
+ tree pending_stmts;
+
/* The post dominance info has to be up-to-date. */
gcc_assert (dom_computed[CDI_POST_DOMINATORS] == DOM_OK);
/* Get the immediate post dominator of bb. */
@@ -739,7 +743,13 @@ remove_dead_stmt (block_stmt_iterator *i
/* Redirect the first edge out of BB to reach POST_DOM_BB. */
redirect_edge_and_branch (EDGE_SUCC (bb, 0), post_dom_bb);
+
+ e = EDGE_SUCC (bb, 0);
+
+ pending_stmts = PENDING_STMT (e);
+
PENDING_STMT (EDGE_SUCC (bb, 0)) = NULL;
+
EDGE_SUCC (bb, 0)->probability = REG_BR_PROB_BASE;
EDGE_SUCC (bb, 0)->count = bb->count;
@@ -755,6 +765,76 @@ remove_dead_stmt (block_stmt_iterator *i
else
EDGE_SUCC (bb, 0)->flags &= ~EDGE_FALLTHRU;
+ /* Now adjust the PHI args for the new edge in the new dest. */
+ for (phi = phi_nodes (e->dest);
+ phi;
+ phi = PHI_CHAIN (phi))
+ {
+ tree arg = NULL;
+ tree *pendp = &pending_stmts;
+ tree phi1;
+ edge e1;
+ edge_iterator ei;
+
+ /* If it's already set for a variable, we're done! */
+ if (PHI_ARG_DEF (phi, e->dest_idx))
+ continue;
+
+ /* Try to locate a value in the list of PHI args collected
+ while removing the old edge. */
+ while (*pendp)
+ {
+ if (SSA_NAME_VAR (PHI_RESULT (phi))
+ == SSA_NAME_VAR (TREE_PURPOSE (*pendp)))
+ {
+ arg = TREE_VALUE (*pendp);
+ *pendp = TREE_CHAIN (*pendp);
+ break;
+ }
+ pendp = &TREE_CHAIN (*pendp);
+ }
+
+ if (arg)
+ {
+ add_phi_arg (phi, arg, e);
+ continue;
+ }
+
+ /* As a last resort, we can try to find ssa args in the
+ other outgoing edges of the src block. */
+ FOR_EACH_EDGE (e1, ei, bb->succs)
+ {
+ if (e1 == e)
+ continue;
+
+ for (phi1 = phi_nodes (e1->dest); phi1;
+ phi1 = PHI_CHAIN (phi1))
+ {
+ if (SSA_NAME_VAR (PHI_RESULT (phi1))
+ == SSA_NAME_VAR (PHI_RESULT (phi)))
+ {
+ arg = PHI_ARG_DEF (phi1, e1->dest_idx);
+ break;
+ }
+ }
+
+ if (arg)
+ break;
+ }
+
+ if (arg)
+ {
+ add_phi_arg (phi, arg, e);
+ continue;
+ }
+
+ /* There's a slight possibility that we won't have been able
+ to find a PHI arg in any of the previously-existing
+ outgoing edges. Should this ever happen, we'd have to
+ scan the BB or its preds for a definition. */
+ gcc_unreachable ();
+ }
+
/* Remove the remaining the outgoing edges. */
while (!single_succ_p (bb))
remove_edge (EDGE_SUCC (bb, 1));
Index: gcc/testsuite/ChangeLog
from Alexandre Oliva <[EMAIL PROTECTED]>
PR tree-optimization/20640
* gcc.dg/torture/tree-loop-1.c: New.
Index: gcc/testsuite/gcc.dg/torture/tree-loop-1.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/torture/tree-loop-1.c
diff -N gcc/testsuite/gcc.dg/torture/tree-loop-1.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/torture/tree-loop-1.c 30 Mar 2005 05:28:22 -0000
@@ -0,0 +1,21 @@
+/* PR tree-optimization/20640 */
+
+/* After unrolling the loop, we'd turn some conditional branches into
+ unconditional ones, but branch redirection would fail to compute
+ the PHI args for the PHI nodes in the replacement edge
+ destination, so they'd remain NULL causing crashes later on. */
+
+/* { dg-do compile } */
+
+static int a = 0;
+extern int foo (void);
+extern int *bar (void) __attribute__ ((__const__));
+
+void
+test (int x)
+{
+ int b = 10;
+ while (foo () == -1 && *bar () == 4 && b > 0)
+ --b;
+ a = x;
+}
--
Alexandre Oliva http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer [EMAIL PROTECTED], gcc.gnu.org}
Free Software Evangelist [EMAIL PROTECTED], gnu.org}
--
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20640
