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


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.

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.

jeff


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