Re: PR61385: phiopt drops some PHIs

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Tue, Jun 3, 2014 at 10:30 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Tue, 3 Jun 2014, Richard Biener wrote:
>
>> 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?),
>
>
> Yes, there is a single incoming edge to middle-bb.
>
>
>> so it's way better to re-materialize the PHI as a
>> gimple assignment at the start of the basic block.
>
>
> I thought there might be a reason why it was a PHI and not an assignment so
> I wasn't sure doing that would be ok. It is indeed much easier, so I'll do
> that...
Maybe it's leftover of loop closed ssa form, but I guess it's useless
at the stage of phiopt.

Thanks,
bin
>
>
>> If they are
>> singletons (which I expect), the easiest way is to propagate their
>> single argument into all uses and simply remove them.
>
>
> ... or indeed that, now that I have found a function called replace_uses_by
> which should do exactly what I need.
>
> Thanks,
>
> --
> Marc Glisse



-- 
Best Regards.