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

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

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.
>
> But I'm a big believer in catching as many CFG simplifications as early as
> we can as they tend to have nice cascading effects.  So if we can pick it up
> by being smarter in how we duplicate arguments, then I'm all for it.
>
>> +  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.

Thanks,
bin
>
> jeff



-- 
Best Regards.