On Tue, Jun 3, 2014 at 10:30 PM, Marc Glisse <marc.gli...@inria.fr> wrote:
> On Tue, 3 Jun 2014, Richard Biener wrote:
>
>> On Tue, Jun 3, 2014 at 3:48 PM, Marc Glisse <marc.gli...@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.gli...@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.
