This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH GCC]Fix pr60363 by adding backtraced value of phi arg along jump threading path
- From: "Bin.Cheng" <amker dot cheng at gmail dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: "bin.cheng" <bin dot cheng at arm dot com>, gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 17 Apr 2014 18:07:47 +0800
- Subject: Re: [PATCH GCC]Fix pr60363 by adding backtraced value of phi arg along jump threading path
- Authentication-results: sourceware.org; auth=none
- References: <006901cf4292$a78407e0$f68c17a0$ at arm dot com> <534F66DF dot 6060701 at redhat 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.