Handle degenerated PHI nodes for entry edge in expansion (PR rtl-optimization/47028)

Submitted by Jakub Jelinek on Jan. 1, 2011, 6:30 p.m.

Details

Message ID 20110101183057.GY16156@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Jan. 1, 2011, 6:30 p.m.
Hi!

On the following testcase we end up with a degenerate PHI node for the entry
edge, which results in the insns being inserted on that edge, which is
wrong, because the insns depend on parameter setup.
The following patch fixes it by inserting those after parm_birth_insn.
Another alternative, mentioned by richi in the PR, would be to get rid of
degenerate PHI nodes during cfg cleanups, haven't tried that yet.

2011-01-01  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/47028
	* cfgexpand.c (gimple_expand_cfg): Insert entry edge
	insertions after parm_birth_insn instead of at the beginning
	of first bb.

	* gcc.dg/pr47028.c: New test.


	Jakub

Comments

Richard Guenther Jan. 2, 2011, 4:13 p.m.
On Sat, Jan 1, 2011 at 7:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On the following testcase we end up with a degenerate PHI node for the entry
> edge, which results in the insns being inserted on that edge, which is
> wrong, because the insns depend on parameter setup.
> The following patch fixes it by inserting those after parm_birth_insn.
> Another alternative, mentioned by richi in the PR, would be to get rid of
> degenerate PHI nodes during cfg cleanups, haven't tried that yet.
>
> 2011-01-01  Jakub Jelinek  <jakub@redhat.com>
>
>        PR rtl-optimization/47028
>        * cfgexpand.c (gimple_expand_cfg): Insert entry edge
>        insertions after parm_birth_insn instead of at the beginning
>        of first bb.
>
>        * gcc.dg/pr47028.c: New test.
>
> --- gcc/cfgexpand.c.jj  2010-12-28 12:32:52.000000000 +0100
> +++ gcc/cfgexpand.c     2010-12-30 16:36:01.000000000 +0100
> @@ -4085,7 +4085,19 @@ gimple_expand_cfg (void)
>       for (ei = ei_start (bb->succs); (e = ei_safe_edge (ei)); )
>        {
>          if (e->insns.r)
> -           commit_one_edge_insertion (e);
> +           {
> +             /* Avoid putting insns before parm_birth_insn.  */
> +             if (e->src == ENTRY_BLOCK_PTR
> +                 && single_succ_p (ENTRY_BLOCK_PTR)

Isn't that always true?

Ok anyway.

Thanks,
Richard.

> +                 && parm_birth_insn)
> +               {
> +                 rtx insns = e->insns.r;
> +                 e->insns.r = NULL_RTX;
> +                 emit_insn_after_noloc (insns, parm_birth_insn, e->dest);
> +               }
> +             else
> +               commit_one_edge_insertion (e);
> +           }
>          else
>            ei_next (&ei);
>        }
> --- gcc/testsuite/gcc.dg/pr47028.c.jj   2010-12-30 16:55:39.000000000 +0100
> +++ gcc/testsuite/gcc.dg/pr47028.c      2010-12-30 13:26:57.000000000 +0100
> @@ -0,0 +1,19 @@
> +/* PR rtl-optimization/47028 */
> +/* { dg-do run } */
> +/* { dg-options "-O -foptimize-sibling-calls -fno-forward-propagate -fno-tree-copy-prop -fno-tree-dominator-opts" } */
> +
> +int
> +fib (int n)
> +{
> +  if (n <= 1)
> +    return 1;
> +  return fib (n - 2) + fib (n - 1);
> +}
> +
> +int
> +main (void)
> +{
> +  if (fib (5) != 8)
> +    __builtin_abort ();
> +  return 0;
> +}
>
>        Jakub
>
Paolo Bonzini Jan. 3, 2011, 12:14 p.m.
On 01/01/2011 07:30 PM, Jakub Jelinek wrote:
> Hi!
>
> On the following testcase we end up with a degenerate PHI node for the entry
> edge, which results in the insns being inserted on that edge, which is
> wrong, because the insns depend on parameter setup.
> The following patch fixes it by inserting those after parm_birth_insn.
> Another alternative, mentioned by richi in the PR, would be to get rid of
> degenerate PHI nodes during cfg cleanups, haven't tried that yet.

Are you sure this cannot happen in other commit_one_edge_insertion callers?

Paolo
Jakub Jelinek Jan. 4, 2011, 12:47 p.m.
On Mon, Jan 03, 2011 at 01:14:12PM +0100, Paolo Bonzini wrote:
> On 01/01/2011 07:30 PM, Jakub Jelinek wrote:
> >Hi!
> >
> >On the following testcase we end up with a degenerate PHI node for the entry
> >edge, which results in the insns being inserted on that edge, which is
> >wrong, because the insns depend on parameter setup.
> >The following patch fixes it by inserting those after parm_birth_insn.
> >Another alternative, mentioned by richi in the PR, would be to get rid of
> >degenerate PHI nodes during cfg cleanups, haven't tried that yet.
> 
> Are you sure this cannot happen in other commit_one_edge_insertion callers?

Some other edge insertions actually rely on insns being added at the
beginning of the initial bb rather than after parm_birth_insn (e.g. when
function.c adds prologue).  That's why I haven't changed it in
commit_one_edge_insertion.

	Jakub

Patch hide | download patch | download mbox

--- gcc/cfgexpand.c.jj	2010-12-28 12:32:52.000000000 +0100
+++ gcc/cfgexpand.c	2010-12-30 16:36:01.000000000 +0100
@@ -4085,7 +4085,19 @@  gimple_expand_cfg (void)
       for (ei = ei_start (bb->succs); (e = ei_safe_edge (ei)); )
 	{
 	  if (e->insns.r)
-	    commit_one_edge_insertion (e);
+	    {
+	      /* Avoid putting insns before parm_birth_insn.  */
+	      if (e->src == ENTRY_BLOCK_PTR
+		  && single_succ_p (ENTRY_BLOCK_PTR)
+		  && parm_birth_insn)
+		{
+		  rtx insns = e->insns.r;
+		  e->insns.r = NULL_RTX;
+		  emit_insn_after_noloc (insns, parm_birth_insn, e->dest);
+		}
+	      else
+		commit_one_edge_insertion (e);
+	    }
 	  else
 	    ei_next (&ei);
 	}
--- gcc/testsuite/gcc.dg/pr47028.c.jj	2010-12-30 16:55:39.000000000 +0100
+++ gcc/testsuite/gcc.dg/pr47028.c	2010-12-30 13:26:57.000000000 +0100
@@ -0,0 +1,19 @@ 
+/* PR rtl-optimization/47028 */
+/* { dg-do run } */
+/* { dg-options "-O -foptimize-sibling-calls -fno-forward-propagate -fno-tree-copy-prop -fno-tree-dominator-opts" } */
+
+int
+fib (int n)
+{
+  if (n <= 1)
+    return 1;
+  return fib (n - 2) + fib (n - 1);
+}
+
+int
+main (void)
+{
+  if (fib (5) != 8)
+    __builtin_abort ();
+  return 0;
+}