diff mbox

update address taken: don't drop clobbers

Message ID alpine.DEB.2.11.1410171212370.2952@stedding.saclay.inria.fr
State New
Headers show

Commit Message

Marc Glisse Oct. 17, 2014, 8:41 p.m. UTC
On Thu, 16 Oct 2014, Jeff Law wrote:

>> BTW, I dislike having multiple DCE implementations...
> Similarly.  The proposal above was just to determine if we should schedule 
> DCE or not.

Thinking about it some more, I don't think we should need any kind of DCE 
here. The rewriting in update_ssa already does a form of forward 
propagation that avoids generating dead assignments, the problem only 
occurs if we explicitly introduce this new assignment. So I believe we 
should go back to an earlier version, like the attached, which is less 
work for the compiler.

And now I can go re-read the old discussion (apparently I should avoid 
gsi_replace, and there may be other ways to handle the coalescing).

Comments

Jeff Law Oct. 24, 2014, 8:21 p.m. UTC | #1
On 10/17/14 14:41, Marc Glisse wrote:
> On Thu, 16 Oct 2014, Jeff Law wrote:
>
>>> BTW, I dislike having multiple DCE implementations...
>> Similarly.  The proposal above was just to determine if we should
>> schedule DCE or not.
>
> Thinking about it some more, I don't think we should need any kind of
> DCE here. The rewriting in update_ssa already does a form of forward
> propagation that avoids generating dead assignments, the problem only
> occurs if we explicitly introduce this new assignment. So I believe we
> should go back to an earlier version, like the attached, which is less
> work for the compiler.
>
> And now I can go re-read the old discussion (apparently I should avoid
> gsi_replace, and there may be other ways to handle the coalescing).
>
I'm starting to agree -- a later message indicated you wanted to drop 
the unlink_stmt_vdef call and you wanted to avoid gsi_replace, that 
seems fine.  I'll approve once those things are taken care of.

jeff
Marc Glisse Oct. 25, 2014, 7:48 a.m. UTC | #2
(replying to both messages)

On Fri, 24 Oct 2014, Jeff Law wrote:

[ https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ]
> 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?

Without the liveness patch, their lifetime should mean that they don't 
coalesce with anything. But I would expect they get their own partition 
then (I am forgetting these details way too fast...). With the liveness 
patch which gives them an empty lifetime, they should all coalesce with at 
least one other ssa_name.

> 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.

As I mentioned in my message, I don't know. Last time I looked I couldn't 
find how coalescing was actually performed. tree-outof-ssa.c has a 
function rewrite_trees with a promising comment but an empty body :-/
I agree that we need to understand what happens at expansion time when the 
variables are not coalesced before pushing a patch that prevents 
coalescing. I was kind of hoping someone would have a pointer...


On Fri, 24 Oct 2014, Jeff Law wrote:

[ https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html ]
> On 10/17/14 14:41, Marc Glisse wrote:
>> 
>> Thinking about it some more, I don't think we should need any kind of
>> DCE here. The rewriting in update_ssa already does a form of forward
>> propagation that avoids generating dead assignments, the problem only
>> occurs if we explicitly introduce this new assignment. So I believe we
>> should go back to an earlier version, like the attached, which is less
>> work for the compiler.
>> 
>> And now I can go re-read the old discussion (apparently I should avoid
>> gsi_replace, and there may be other ways to handle the coalescing).
>> 
> I'm starting to agree -- a later message indicated you wanted to drop the 
> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine. 
> I'll approve once those things are taken care of.

I don't really want to avoid gsi_replace, but I am willing to do it if it 
makes reviewers nervous to call such a high-level function in the cleanup 
code between passes (Richard in particular was unhappy about it).

To clarify things so I know what to test and re-post, we are talking 
about the patch in 
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html and we can forget 
about the coalescing thing in 
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ?
(I'd be happy with that :-)

Thanks,
Jeff Law Oct. 31, 2014, 9 p.m. UTC | #3
On 10/25/14 01:48, Marc Glisse wrote:
>
> Without the liveness patch, their lifetime should mean that they don't
> coalesce with anything. But I would expect they get their own partition
> then (I am forgetting these details way too fast...). With the liveness
> patch which gives them an empty lifetime, they should all coalesce with
> at least one other ssa_name.
>
>> 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.
>
> As I mentioned in my message, I don't know. Last time I looked I
> couldn't find how coalescing was actually performed. tree-outof-ssa.c
> has a function rewrite_trees with a promising comment but an empty body :-/
> I agree that we need to understand what happens at expansion time when
> the variables are not coalesced before pushing a patch that prevents
> coalescing. I was kind of hoping someone would have a pointer...
Matz changed all the out-of-ssa stuff a while back.  Basically instead 
of actually rewriting things to reflect going out of SSA, we just record 
the partitions and refer back to the partitions representative element 
during expansion.

So all we really need to do is verify reasonable partitions are built 
and that should answer any questions around expansion.


> I don't really want to avoid gsi_replace, but I am willing to do it if
> it makes reviewers nervous to call such a high-level function in the
> cleanup code between passes (Richard in particular was unhappy about it).
Ah, I thought it was something you'd wanted, but sounds like it was 
Richi's request.


>
> To clarify things so I know what to test and re-post, we are talking
> about the patch in
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html and we can
> forget about the coalescing thing in
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ?
> (I'd be happy with that :-)
Yes, those are the ones I was referring to.

jeff
diff mbox

Patch

Index: testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/pr60770-1.c	(revision 0)
+++ testsuite/gcc.dg/tree-ssa/pr60770-1.c	(working copy)
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+  int*p;
+  {
+    int yyy=n;
+    p=&yyy;
+  }
+  return *p; /* { dg-warning "yyy" } */
+}
Index: tree-into-ssa.c
===================================================================
--- tree-into-ssa.c	(revision 216384)
+++ tree-into-ssa.c	(working copy)
@@ -1837,26 +1837,35 @@  maybe_register_def (def_operand_p def_p,
 {
   tree def = DEF_FROM_PTR (def_p);
   tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
 
   /* If DEF is a naked symbol that needs renaming, create a new
      name for it.  */
   if (marked_for_renaming (sym))
     {
       if (DECL_P (def))
 	{
-	  tree tracked_var;
-
-	  def = make_ssa_name (def, stmt);
+	  if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+	    {
+	      /* Replace clobber stmts with a default def. This new use of a
+		 default definition may make it look like SSA_NAMEs have
+		 conflicting lifetimes, so we need special code to let them
+		 coalesce properly.  */
+	      unlink_stmt_vdef (stmt);
+	      gsi_replace (&gsi, gimple_build_nop (), true);
+	      def = get_or_create_ssa_default_def (cfun, sym);
+	    }
+	  else
+	    def = make_ssa_name (def, stmt);
 	  SET_DEF (def_p, def);
 
-	  tracked_var = target_for_debug_bind (sym);
+	  tree tracked_var = target_for_debug_bind (sym);
 	  if (tracked_var)
 	    {
 	      gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
 	      /* If stmt ends the bb, insert the debug stmt on the single
 		 non-EH edge from the stmt.  */
 	      if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
 		{
 		  basic_block bb = gsi_bb (gsi);
 		  edge_iterator ei;
 		  edge e, ef = NULL;
Index: tree-ssa-live.c
===================================================================
--- tree-ssa-live.c	(revision 216384)
+++ tree-ssa-live.c	(working copy)
@@ -40,20 +40,21 @@  along with GCC; see the file COPYING3.
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "expr.h"
 #include "tree-dfa.h"
 #include "timevar.h"
 #include "dumpfile.h"
 #include "tree-ssa-live.h"
 #include "diagnostic-core.h"
 #include "debug.h"
 #include "flags.h"
+#include "tree-ssa.h"
 
 #ifdef ENABLE_CHECKING
 static void  verify_live_on_entry (tree_live_info_p);
 #endif
 
 
 /* VARMAP maintains a mapping from SSA version number to real variables.
 
    All SSA_NAMES are divided into partitions.  Initially each ssa_name is the
    only member of it's own partition.  Coalescing will attempt to group any
@@ -1086,20 +1087,24 @@  set_var_live_on_entry (tree ssa_name, tr
   if (stmt)
     {
       def_bb = gimple_bb (stmt);
       /* Mark defs in liveout bitmap temporarily.  */
       if (def_bb)
 	bitmap_set_bit (&live->liveout[def_bb->index], p);
     }
   else
     def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
 
+  /* An undefined local variable does not need to be very alive.  */
+  if (ssa_undefined_value_p (ssa_name, false))
+    return;
+
   /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
      add it to the list of live on entry blocks.  */
   FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
     {
       gimple use_stmt = USE_STMT (use);
       basic_block add_block = NULL;
 
       if (gimple_code (use_stmt) == GIMPLE_PHI)
         {
 	  /* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1427,25 @@  verify_live_on_entry (tree_live_info_p l
 			  fprintf (stderr, "\n");
 			}
 		      else
 			fprintf (stderr, " and there is no default def.\n");
 		    }
 		}
 	    }
 	  else
 	    if (d == var)
 	      {
+		/* An undefined local variable does not need to be very
+		   alive.  */
+		if (ssa_undefined_value_p (var, false))
+		  continue;
+
 		/* The only way this var shouldn't be marked live on entry is
 		   if it occurs in a PHI argument of the block.  */
 		size_t z;
 		bool ok = false;
 		gimple_stmt_iterator gsi;
 		for (gsi = gsi_start_phis (e->dest);
 		     !gsi_end_p (gsi) && !ok;
 		     gsi_next (&gsi))
 		  {
 		    gimple phi = gsi_stmt (gsi);
Index: tree-ssa.c
===================================================================
--- tree-ssa.c	(revision 216384)
+++ tree-ssa.c	(working copy)
@@ -1178,24 +1178,25 @@  tree_ssa_useless_type_conversion (tree e
 
 tree
 tree_ssa_strip_useless_type_conversions (tree exp)
 {
   while (tree_ssa_useless_type_conversion (exp))
     exp = TREE_OPERAND (exp, 0);
   return exp;
 }
 
 
-/* Return true if T, an SSA_NAME, has an undefined value.  */
+/* Return true if T, an SSA_NAME, has an undefined value.  PARTIAL is what
+   should be returned if the value is only partially undefined.  */
 
 bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
 {
   gimple def_stmt;
   tree var = SSA_NAME_VAR (t);
 
   if (!var)
     ;
   /* Parameters get their initial value from the function entry.  */
   else if (TREE_CODE (var) == PARM_DECL)
     return false;
   /* When returning by reference the return address is actually a hidden
@@ -1205,21 +1206,21 @@  ssa_undefined_value_p (tree t)
   /* Hard register variables get their initial value from the ether.  */
   else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
     return false;
 
   /* The value is undefined iff its definition statement is empty.  */
   def_stmt = SSA_NAME_DEF_STMT (t);
   if (gimple_nop_p (def_stmt))
     return true;
 
   /* Check if the complex was not only partially defined.  */
-  if (is_gimple_assign (def_stmt)
+  if (partial && is_gimple_assign (def_stmt)
       && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
     {
       tree rhs1, rhs2;
 
       rhs1 = gimple_assign_rhs1 (def_stmt);
       rhs2 = gimple_assign_rhs2 (def_stmt);
       return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
 	     || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
     }
   return false;
@@ -1551,32 +1552,20 @@  execute_update_addresses_taken (void)
 		rhs = gimple_assign_rhs1 (stmt);
 		if (gimple_assign_lhs (stmt) != lhs
 		    && !useless_type_conversion_p (TREE_TYPE (lhs),
 						   TREE_TYPE (rhs)))
 		  rhs = fold_build1 (VIEW_CONVERT_EXPR,
 				     TREE_TYPE (lhs), rhs);
 
 		if (gimple_assign_lhs (stmt) != lhs)
 		  gimple_assign_set_lhs (stmt, lhs);
 
-		/* For var ={v} {CLOBBER}; where var lost
-		   TREE_ADDRESSABLE just remove the stmt.  */
-		if (DECL_P (lhs)
-		    && TREE_CLOBBER_P (rhs)
-		    && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
-		  {
-		    unlink_stmt_vdef (stmt);
-      		    gsi_remove (&gsi, true);
-		    release_defs (stmt);
-		    continue;
-		  }
-
 		if (gimple_assign_rhs1 (stmt) != rhs)
 		  {
 		    gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
 		    gimple_assign_set_rhs_from_tree (&gsi, rhs);
 		  }
 	      }
 
 	    else if (gimple_code (stmt) == GIMPLE_CALL)
 	      {
 		unsigned i;
Index: tree-ssa.h
===================================================================
--- tree-ssa.h	(revision 216384)
+++ tree-ssa.h	(working copy)
@@ -44,21 +44,21 @@  extern tree target_for_debug_bind (tree)
 extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
 extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
 extern void reset_debug_uses (gimple);
 extern void release_defs_bitset (bitmap toremove);
 extern void verify_ssa (bool, bool);
 extern void init_tree_ssa (struct function *);
 extern void delete_tree_ssa (void);
 extern bool tree_ssa_useless_type_conversion (tree);
 extern tree tree_ssa_strip_useless_type_conversions (tree);
 
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
 extern void execute_update_addresses_taken (void);
 
 /* Given an edge_var_map V, return the PHI arg definition.  */
 
 static inline tree
 redirect_edge_var_map_def (edge_var_map *v)
 {
   return v->def;
 }