[PATCH GCC]Fix pr60363 by adding backtraced value of phi arg along jump threading path

[Jeff Law]

On 04/17/14 04:07, Bin.Cheng wrote:
> On Thu, Apr 17, 2014 at 1:30 PM, Jeff Law <law@redhat.com> wrote:
>> On 03/18/14 04:13, bin.cheng wrote:
>>>
>>> Hi,
>>> After control flow graph change made by
>>> http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01492.html, case
>>> gcc.dg/tree-ssa/ssa-dom-thread-4.c is broken on logical_op_short_circuit
>>> targets including cortex-m3/cortex-m0.
>>> The regression reveals a missed opportunity in jump threading, which
>>> causes
>>> a forward basic block doesn't get removed in cfgcleanup after jump
>>> threading
>>> in VRP1.  Root cause is stated at the corresponding PR:
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60363, please refer to it for
>>> detailed report.
>>>
>>> This patch fixes the issue by adding constant value instead of ssa_name as
>>> the new phi argument.  Bootstrap and test on x86_64, also test on
>>> cortex-m3
>>> and the regression is gone.
>>> I think this should wait for stage1, but would like to hear some comments
>>> now.  So does it look reasonable?
>>>
>>>
>>> 2014-03-18  Bin Cheng<bin.cheng@arm.com>
>>>
>>>          PR regression/60363
>>>          * gcc/tree-ssa-threadupdate.c (get_value_locus_in_path): New.
>>>          (copy_phi_args): New parameters.  Call get_value_locus_in_path.
>>>          (update_destination_phis): New parameter.
>>>          (create_edge_and_update_destination_phis): Ditto.
>>>          (ssa_fix_duplicate_block_edges): Pass new arguments.
>>>          (thread_single_edge): Ditto.
>>
>> This is a good and interesting catch. DOM knows how to propagate these
>> context sensitive equivalences which should expose the optimizable forwarder
>> blocks.
> At the time I was looking into the problem, DOM couldn't understand
> the equivalence.  Maybe it can be improved too.
That's something I improved during 4.9 stage1 development.  There were 
cases where we clearly missed propagating a context sensitive 
equivalence into PHI nodes.  I didn't check this specific case, but it 
looks reasonably similar to the cases I fixed a few months back.

>>> +  for (int j = idx - 1; j >= 0; j--)
>>> +    {
>>> +      edge e = (*path)[j]->e;
>>> +      if (e->dest == def_bb)
>>> +       {
>>> +         arg = gimple_phi_arg_def (def_phi, e->dest_idx);
>>> +         *locus = gimple_phi_arg_location (def_phi, e->dest_idx);
>>> +         return (TREE_CODE (arg) == INTEGER_CST ? arg : def);
>>
>> Presumably any constant that can legitimately appear in a PHI node is good
>> here.  So for example ADDR_EXPR <something in static storage> ought to be
>> handled as well.
>>
>> One could also argue that we should go ahead and do a context sensitive copy
>> propagation here too if ARG turns out to be an SSA_NAME.  You have to be a
>> bit more careful with those and use may_propagate_copy_p and you'd probably
>> want to test the loop depth of the SSA_NAMEs to ensure you're not doing a
>> propagation that is going to muck up LICM.  See loop_depth_of_name uses in
>> tree-ssa-dom.c.
>>
>> Overall I think it's good.  We just need to resolve whether or not we want
>> to catch constant ADDR_EXPRs and/or do the context sensitive copy
>> propagations.
> Do you mean const/copy propagation in jump threading optimization, or
> just an independent opt somewhere else?  It's naturally flow sensitive
> along jump threading path, which looks interesting to me.
Thinking more about it, I'd probably hold off on the context sensitive 
copy propagation right now since you have to check a variety of things 
to ensure correctness and performance.  Handling that as a follow-up 
would make sense if you're interested.

So for this patch to go forward, all I think you need to do is change 
the test TREE_CODE (arg) == INTEGER_CST above to instead test 
is_gimple_min_invariant (arg), bootstrap & test.  Assuming the bootstrap 
and regressino test pass with that change, consider the patch pre-approved.

jeff