On 04/17/14 04:07, Bin.Cheng wrote:
On Thu, Apr 17, 2014 at 1:30 PM, Jeff Law <l...@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.ch...@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
