This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Friday, April 25, 2014 4:53 AM
> To: Bin.Cheng
> Cc: Bin Cheng; gcc-patches List
> Subject: Re: [PATCH GCC]Fix pr60363 by adding backtraced value of phi arg
> along jump threading path
> 
> 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.

Hi,
I updated and rebased the patch against latest trunk.  It passes bootstrap
and regression test on x86/x86_64.   Also pr60363 is fixed on
logical_op_short_circuit targets.  Is it OK?

Since ssa-dom-thread-4.c is fixed now, I also reverted the XFAIL test for
it.

An irrelevant point, the const propagation opportunities exist not only for
threaded jump path, but also for the original path after threading jump.  So
maybe it's worth to do the general transformation as Jeff suggested before.

Thanks,
bin


2014-05-04  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.

gcc/testsuite/ChangeLog
2014-05-04  Bin Cheng  <bin.cheng@arm.com>

	PR target/60363
	* gcc.target/tree-ssa/ssa-dom-thread-4.c: Revert XFAIL test.

Attachment: pr60363-20140504.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]