diff mbox

update address taken: don't drop clobbers

Message ID alpine.DEB.2.11.1410182108070.8744@laptop-mg.saclay.inria.fr
State New
Headers show

Commit Message

Marc Glisse Oct. 18, 2014, 9:59 p.m. UTC 
On Thu, 10 Jul 2014, Richard Biener wrote:

> On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>
>> we currently drop clobbers on variables whose address is not taken anymore.
>> However, rewrite_stmt has code to replace them with an SSA_NAME with a
>> default definition (an uninitialized variable), and I believe
>> rewrite_update_stmt should do the same. This allows us to warn sometimes
>> (see testcase), but during the debugging I also noticed several places where
>> it allowed CCP to simplify further PHIs, so this is also an optimization.
>>
>> In an earlier version of the patch, I was using
>> get_or_create_ssa_default_def (cfun, sym);
>> (I was reusing the same variable). This passed bootstrap+testsuite on all
>> languages except for ada. Indeed, the compiler wanted to coalesce several
>> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There are
>> abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing an
>> undefined ssa_name, maybe something should have prevented us from reaching
>> such a situation, but creating a new variable was the simplest workaround.
>
> Hmm.  We indeed notice "late" that the new names are used in abnormal
> PHIs.  Note that usually rewriting a symbol into SSA form does not
> create overlapping life-ranges - but of course you are possibly introducing
> those by the new use of the default definitions.
>
> Apart from the out-of-SSA patch you proposed elsewhere a possibility
> is to simply never mark undefined SSA names as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
> as must-coalesce).

For "not mark those as must-coalesce", replacing the liveness patch with 
the attached patch also passed the testsuite: I skip undefined variables 
when handling must-coalesce, and let the regular coalescing code handle 
them. I am not sure what happens during expansion though, and bootstrap 
only hits this issue a couple times in ada so it doesn't prove much.

This patch doesn't conflict with the liveness patch, they are rather 
complementary. I didn't test but I am quite confident that having both 
patches would also pass bootstrap+testsuite.

Of course that all becomes unnecessary if we use default definitions of 
new variables instead of always the same old variable, but I can 
understand not wanting all those new artificial variables.

I would be ok with the patch at 
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html
(minus the line with unlink_stmt_vdef, which is indeed unnecessary)
(I looked at what gsi_replace does when replacing a clobber by a nop, and 
it seems harmless, but I can manually inline the non-dead parts of it if 
you want)

Comments

Jeff Law Oct. 24, 2014, 8:18 p.m. UTC | #1 
On 10/18/14 15:59, Marc Glisse wrote:
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>> On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>>
>>> we currently drop clobbers on variables whose address is not taken
>>> anymore.
>>> However, rewrite_stmt has code to replace them with an SSA_NAME with a
>>> default definition (an uninitialized variable), and I believe
>>> rewrite_update_stmt should do the same. This allows us to warn sometimes
>>> (see testcase), but during the debugging I also noticed several
>>> places where
>>> it allowed CCP to simplify further PHIs, so this is also an
>>> optimization.
>>>
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all
>>> languages except for ada. Indeed, the compiler wanted to coalesce
>>> several
>>> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't.
>>> There are
>>> abnormal PHIs involved. Maybe it shouldn't have insisted on
>>> coalescing an
>>> undefined ssa_name, maybe something should have prevented us from
>>> reaching
>>> such a situation, but creating a new variable was the simplest
>>> workaround.
>>
>> Hmm.  We indeed notice "late" that the new names are used in abnormal
>> PHIs.  Note that usually rewriting a symbol into SSA form does not
>> create overlapping life-ranges - but of course you are possibly
>> introducing
>> those by the new use of the default definitions.
>>
>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>> is to simply never mark undefined SSA names as
>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>> as must-coalesce).
>
> For "not mark those as must-coalesce", replacing the liveness patch with
> the attached patch also passed the testsuite: I skip undefined variables
> when handling must-coalesce, and let the regular coalescing code handle
> them. I am not sure what happens during expansion though, and bootstrap
> only hits this issue a couple times in ada so it doesn't prove much.
>
> This patch doesn't conflict with the liveness patch, they are rather
> complementary. I didn't test but I am quite confident that having both
> patches would also pass bootstrap+testsuite.
>
> Of course that all becomes unnecessary if we use default definitions of
> new variables instead of always the same old variable, but I can
> understand not wanting all those new artificial variables.
>
> I would be ok with the patch at
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html
> (minus the line with unlink_stmt_vdef, which is indeed unnecessary)
> (I looked at what gsi_replace does when replacing a clobber by a nop,
> and it seems harmless, but I can manually inline the non-dead parts of
> it if you want)
So I'm still trying to get comfortable with this patch.  I guess my 
concerns about having one of the undefined value SSA_NAMEs appearing in 
two conflicting coalesce lists are alleviated by the twiddle to 
coalesce_partitions where we essentially ignore them.

So in the end, they don't end up a part of any partition?  What happens 
when we expand them?  I guess they get a new pseudo since they're a 
distinct partition?  If we had a sensible story for expansion, then I 
could probably get on board with this patch.

And I'm still going to look at the other as well -- as you mention, 
they're independent.

jeff
diff mbox

Patch

Index: tree-ssa-coalesce.c
===================================================================
--- tree-ssa-coalesce.c	(revision 216415)
+++ tree-ssa-coalesce.c	(working copy)
@@ -36,20 +36,21 @@  along with GCC; see the file COPYING3.
 #include "gimple.h"
 #include "gimple-iterator.h"
 #include "gimple-ssa.h"
 #include "tree-phinodes.h"
 #include "ssa-iterators.h"
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "tree-ssa-live.h"
 #include "tree-ssa-coalesce.h"
 #include "diagnostic-core.h"
+#include "tree-ssa.h"
 
 
 /* This set of routines implements a coalesce_list.  This is an object which
    is used to track pairs of ssa_names which are desirable to coalesce
    together to avoid copies.  Costs are associated with each pair, and when
    all desired information has been collected, the object can be used to
    order the pairs for processing.  */
 
 /* This structure defines a pair entry.  */
 
@@ -962,20 +963,22 @@  create_outofssa_var_map (coalesce_list_p
 		  saw_copy = true;
 		  bitmap_set_bit (used_in_copy, SSA_NAME_VERSION (arg));
 		  if ((e->flags & EDGE_ABNORMAL) == 0)
 		    {
 		      int cost = coalesce_cost_edge (e);
 		      if (cost == 1 && has_single_use (arg))
 			add_cost_one_coalesce (cl, ver, SSA_NAME_VERSION (arg));
 		      else
 			add_coalesce (cl, ver, SSA_NAME_VERSION (arg), cost);
 		    }
+		  else if (ssa_undefined_value_p (arg, false))
+		    add_coalesce (cl, ver, SSA_NAME_VERSION (arg), MUST_COALESCE_COST - 1);
 		}
 	    }
 	  if (saw_copy)
 	    bitmap_set_bit (used_in_copy, ver);
 	}
 
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
         {
 	  stmt = gsi_stmt (gsi);
 
@@ -1189,20 +1192,22 @@  coalesce_partitions (var_map map, ssa_co
       FOR_EACH_EDGE (e, ei, bb->preds)
 	if (e->flags & EDGE_ABNORMAL)
 	  {
 	    gimple_stmt_iterator gsi;
 	    for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi);
 		 gsi_next (&gsi))
 	      {
 		gimple phi = gsi_stmt (gsi);
 		tree res = PHI_RESULT (phi);
 	        tree arg = PHI_ARG_DEF (phi, e->dest_idx);
+		if (ssa_undefined_value_p (arg, false))
+		  continue;
 		int v1 = SSA_NAME_VERSION (res);
 		int v2 = SSA_NAME_VERSION (arg);
 
 		if (debug)
 		  fprintf (debug, "Abnormal coalesce: ");
 
 		if (!attempt_coalesce (map, graph, v1, v2, debug))
 		  fail_abnormal_edge_coalesce (v1, v2);
 	      }
 	  }
@@ -1287,21 +1292,22 @@  coalesce_ssa_name (void)
 		{
 		  /* If the variable is a PARM_DECL or a RESULT_DECL, we
 		     _require_ that all the names originating from it be
 		     coalesced, because there must be a single partition
 		     containing all the names so that it can be assigned
 		     the canonical RTL location of the DECL safely.
 		     If in_lto_p, a function could have been compiled
 		     originally with optimizations and only the link
 		     performed at -O0, so we can't actually require it.  */
 		  const int cost
-		    = (TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL || in_lto_p)
+		    = (TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL || in_lto_p
+		       || ssa_undefined_value_p (a, false))
 		      ? MUST_COALESCE_COST - 1 : MUST_COALESCE_COST;
 		  add_coalesce (cl, SSA_NAME_VERSION (a),
 				SSA_NAME_VERSION (*slot), cost);
 		  bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (a));
 		  bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (*slot));
 		}
 	    }
 	}
     }
   if (dump_file && (dump_flags & TDF_DETAILS))