PR61385: phiopt drops some PHIs

[Richard Biener]

On Tue, Jun 3, 2014 at 3:48 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> apparently it is possible to have a PHI in the middle basic block of
> value_replacement, so I need to move it as well when I move the statement
> and remove the block.
>
> Bootstrap and testsuite on x86_64-linux-gnu (re-running for various reasons
> but they had completed successfully yesterday).
>
> 2014-06-03  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR tree-optimization/61385
> gcc/
>         * tree-ssa-phiopt.c (value_replacement): Copy PHI nodes before
>         removing the basic block.
> gcc/testsuite/
>         * gcc.dg/tree-ssa/pr61385.c: New file.
>
> --
> Marc Glisse
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr61385.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr61385.c     (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr61385.c     (working copy)
> @@ -0,0 +1,43 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define assert(x) if (!(x)) __builtin_abort ()
> +
> +int a, b, c, d, e, f, g;
> +
> +int
> +fn1 ()
> +{
> +  int *h = &c;
> +  for (; c < 1; c++)
> +    {
> +      int *i = &a, *k = &a;
> +      f = 0;
> +      if (b)
> +       return 0;
> +      if (*h)
> +       {
> +         int **j = &i;
> +         *j = 0;
> +         d = 0;
> +       }
> +      else
> +       g = e = 0;
> +      if (*h)
> +       {
> +         int **l = &k;
> +         *l = &g;
> +       }
> +      d &= *h;
> +      assert (k == &a || k);
> +      assert (i);
> +    }
> +  return 0;
> +}
> +
> +int
> +main ()
> +{
> +  fn1 ();
> +  return 0;
> +}
> Index: gcc/tree-ssa-phiopt.c
> ===================================================================
> --- gcc/tree-ssa-phiopt.c       (revision 211178)
> +++ gcc/tree-ssa-phiopt.c       (working copy)
> @@ -877,20 +877,39 @@ value_replacement (basic_block cond_bb,
>            && operand_equal_for_phi_arg_p (rhs2, cond_lhs)
>            && neutral_element_p (code_def, cond_rhs, true))
>           || (arg1 == rhs2
>               && operand_equal_for_phi_arg_p (rhs1, cond_lhs)
>               && neutral_element_p (code_def, cond_rhs, false))
>           || (operand_equal_for_phi_arg_p (arg1, cond_rhs)
>               && (operand_equal_for_phi_arg_p (rhs2, cond_lhs)
>                   || operand_equal_for_phi_arg_p (rhs1, cond_lhs))
>               && absorbing_element_p (code_def, cond_rhs))))
>      {
> +      /* Move potential PHI nodes.  */
> +      gimple_stmt_iterator psi = gsi_start_phis (middle_bb);
> +      while (!gsi_end_p (psi))
> +       {
> +         gimple phi_moving = gsi_stmt (psi);
> +         gimple newphi = create_phi_node (gimple_phi_result (phi_moving),
> +                                          cond_bb);
> +         int nargs = cond_bb->preds->length();
> +         location_t loc = gimple_phi_arg_location (phi_moving, 0);
> +         tree phi_arg = gimple_phi_arg_def (phi_moving, 0);
> +         for (int i = 0; i < nargs; ++i)
> +           {
> +             edge e = (*cond_bb->preds)[i];
> +             add_phi_arg (newphi, phi_arg, e, loc);

All arguments get the same value (and the PHI in middle-bb is surely
a singleton?), so it's way better to re-materialize the PHI as a
gimple assignment at the start of the basic block.  If they are
singletons (which I expect), the easiest way is to propagate their
single argument into all uses and simply remove them.

Richard.

> +           }
> +         update_stmt (newphi);
> +         gsi_remove (&psi, false);
> +       }
> +
>        gsi = gsi_for_stmt (cond);
>        gimple_stmt_iterator gsi_from = gsi_for_stmt (assign);
>        gsi_move_before (&gsi_from, &gsi);
>        replace_phi_edge_with_variable (cond_bb, e1, phi, lhs);
>        return 2;
>      }
>
>    return 0;
>  }
>
>