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

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

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
>

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

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