On Thu, Apr 17, 2014 at 7:30 AM, 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. > > 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.
Simply use is_gimple_min_invariant (arg) ? arg : def > jeff