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

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 1, 2011, 6:30 p.m.
Message ID <20110101183057.GY16156@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/77149/
State New
Headers show

Comments

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
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

--- 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;
+}