Re: update address taken: don't drop clobbers

[Marc Glisse <marc dot glisse at inria dot fr>]

On Fri, 31 Oct 2014, Richard Biener wrote:

> On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> > On Fri, 24 Oct 2014, Jeff Law wrote:
> >
> > > 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.
> >
> >
> > The following passed bootstrap+testsuite. I wasn't sure what exactly a
> > clobber is guaranteed not to have (no histograms for instance? At least I
> > assumed it doesn't throw) so I may have kept some unnecessary calls when I
> > inlined gsi_replace. I'd be happy to remove any you feel is useless.
> >
> > 2014-10-26  Marc Glisse  <marc.glisse@inria.fr>
> >
> >         PR tree-optimization/60770
> > gcc/
> >         * tree-into-ssa.c: Include value-prof.h.
> >         (maybe_register_def): Replace clobbers with default definitions.
> >         * tree-ssa-live.c: Include tree-ssa.h.
> >         (set_var_live_on_entry): Do not mark undefined variables as live.
> >         (verify_live_on_entry): Do not check undefined variables.
> >         * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
> >         of partially undefined variables.
> >         * tree-ssa.c (ssa_undefined_value_p): Likewise.
> >         (execute_update_addresses_taken): Do not drop clobbers.
> >
> > gcc/testsuite/
> >         * gcc.dg/tree-ssa/pr60770-1.c: New file.
> >
> > --
> > Marc Glisse
> >
> > Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> > ===================================================================
> > --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (revision 0)
> > +++ gcc/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: gcc/tree-into-ssa.c
> > ===================================================================
> > --- gcc/tree-into-ssa.c (revision 216689)
> > +++ gcc/tree-into-ssa.c (working copy)
> > @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
> >  #include "expr.h"
> >  #include "tree-dfa.h"
> >  #include "tree-ssa.h"
> >  #include "tree-inline.h"
> >  #include "tree-pass.h"
> >  #include "cfgloop.h"
> >  #include "domwalk.h"
> >  #include "params.h"
> >  #include "diagnostic-core.h"
> >  #include "tree-into-ssa.h"
> > +#include "value-prof.h"
> >
> >  #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
> >
> >  /* This file builds the SSA form for a function as described in:
> >     R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
> >     Computing Static Single Assignment Form and the Control Dependence
> >     Graph. ACM Transactions on Programming Languages and Systems,
> >     13(4):451-490, October 1991.  */
> >
> >  /* Structure to map a variable VAR to the set of blocks that contain
> > @@ -1837,26 +1838,42 @@ 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;
> > +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> I think you know that sym is a gimple-reg as the code previously
> unconditionally generated an SSA name for it.

I checked that in July and it failed:
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01828.html

> > +           {
> > +             /* 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.  */
> > +             /* Hand-inlined version of the following, for safety
> > +                gsi_replace (&gsi, gimple_build_nop (), true);  */
> > +             gimple nop = gimple_build_nop ();
> > +             gimple_set_bb (nop, gsi_bb (gsi));
> > +             gimple_set_bb (stmt, NULL);
> > +             gimple_remove_stmt_histograms (cfun, stmt);
> > +             delink_stmt_imm_use (stmt);
> > +             gsi_set_stmt (&gsi, nop);
>
> Is there any reason for this dance?  I'd rather have maybe_register_def
> return a bool whether to remove the stmt... passing it down to the
> single caller of rewrite_update_stmt which can then gsi_remove the
> stmt.

For more context, my starting point was the code in rewrite_stmt, which
I was trying to port to rewrite_update_stmt (and thus
maybe_register_def):

        if (gimple_clobber_p (stmt)
            && is_gimple_reg (var))
          {
            /* If we rewrite a DECL into SSA form then drop its
               clobber stmts and replace uses with a new default def.  */
            gcc_checking_assert (TREE_CODE (var) == VAR_DECL
                                 && !gimple_vdef (stmt));
            gsi_replace (si, gimple_build_nop (), true);
            register_new_def (get_or_create_ssa_default_def (cfun, var), var);
            break;
          }

I would be happy using the same gsi_replace line, but you said that it
is dangerous because it runs update_stmt (though I don't see what bad
thing may happen when replacing a clobber by a nop), so I tried to do
the same thing as gsi_replace without update_stmt... Though now that I 
re-read https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01656.html it is not 
clear that you are really opposed to the gsi_replace version.

I tried again with gsi_remove, trying to understand why it was failing 
before, and that is because I was calling it in maybe_register_def instead 
of delegating to whoever calls gsi_next.

> > -         def = make_ssa_name (def, stmt);
> > +             def = get_or_create_ssa_default_def (cfun, sym);
>
> I think if 'def' turns out to be a PARM_DECL this does the wrong
> thing (well, not technically wrong...  but maybe unexpected).  Not
> sure if we ever end up with a PARM = {} clobber though.  Maybe
> guard all this with TREE_CODE (def) == VAR_DECL for extra
> safety.

Hmm, you are right. The rewrite_stmt version has

            gcc_checking_assert (TREE_CODE (var) == VAR_DECL && ...

I don't remember exactly why I removed it, maybe because the second part 
of the assertion was failing.

Here is a new version, that passed bootstrap+testsuite on x86_64-linux-gnu.

2014-11-03  Marc Glisse  <marc.glisse@inria.fr>

	PR tree-optimization/60770
gcc/
	* tree-into-ssa.c (rewrite_update_stmt): Return whether the
	statement should be removed.
	(maybe_register_def): Likewise. Replace clobbers with default
	definitions.
	(rewrite_dom_walker::before_dom_children): Remove statement if
	rewrite_update_stmt says so.
	* tree-ssa-live.c: Include tree-ssa.h.
	(set_var_live_on_entry): Do not mark undefined variables as live.
	(verify_live_on_entry): Do not check undefined variables.
	* tree-ssa.h (ssa_undefined_value_p): New parameter for the case
	of partially undefined variables.
	* tree-ssa.c (ssa_undefined_value_p): Likewise.
	(execute_update_addresses_taken): Do not drop clobbers.

gcc/testsuite/
	* gcc.dg/tree-ssa/pr60770-1.c: New file.

--
Marc Glisse

Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(revision 0)
+++ gcc/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: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c	(revision 217007)
+++ gcc/tree-into-ssa.c	(working copy)
@@ -1826,41 +1826,51 @@ maybe_replace_use_in_debug_stmt (use_ope
   if (rdef && rdef != use)
     SET_USE (use_p, rdef);
 
   return rdef != NULL_TREE;
 }
 
 
 /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES
    or OLD_SSA_NAMES, or if it is a symbol marked for renaming,
    register it as the current definition for the names replaced by
-   DEF_P.  */
+   DEF_P.  Returns whether the statement should be removed.  */
 
-static inline void
+static inline bool
 maybe_register_def (def_operand_p def_p, gimple stmt,
 		    gimple_stmt_iterator gsi)
 {
   tree def = DEF_FROM_PTR (def_p);
   tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
+  bool to_delete = false;
 
   /* 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))
+	    {
+	      gcc_checking_assert (TREE_CODE (sym) == VAR_DECL);
+	      /* 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.  */
+	      to_delete = 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;
@@ -1904,40 +1914,42 @@ maybe_register_def (def_operand_p def_p,
       /* If DEF is a new name, register it as a new definition
 	 for all the names replaced by DEF.  */
       if (is_new_name (def))
 	register_new_update_set (def, names_replaced_by (def));
 
       /* If DEF is an old name, register DEF as a new
 	 definition for itself.  */
       if (is_old_name (def))
 	register_new_update_single (def, def);
     }
+
+  return to_delete;
 }
 
 
 /* Update every variable used in the statement pointed-to by SI.  The
    statement is assumed to be in SSA form already.  Names in
    OLD_SSA_NAMES used by SI will be updated to their current reaching
    definition.  Names in OLD_SSA_NAMES or NEW_SSA_NAMES defined by SI
    will be registered as a new definition for their corresponding name
-   in OLD_SSA_NAMES.  */
+   in OLD_SSA_NAMES.  Returns whether STMT should be removed.  */
 
-static void
+static bool
 rewrite_update_stmt (gimple stmt, gimple_stmt_iterator gsi)
 {
   use_operand_p use_p;
   def_operand_p def_p;
   ssa_op_iter iter;
 
   /* Only update marked statements.  */
   if (!rewrite_uses_p (stmt) && !register_defs_p (stmt))
-    return;
+    return false;
 
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
       fprintf (dump_file, "Updating SSA information for statement ");
       print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
     }
 
   /* Rewrite USES included in OLD_SSA_NAMES and USES whose underlying
      symbol is marked for renaming.  */
   if (rewrite_uses_p (stmt))
@@ -1974,23 +1986,26 @@ rewrite_update_stmt (gimple stmt, gimple
       else
 	{
 	  FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
 	    maybe_replace_use (use_p);
 	}
     }
 
   /* Register definitions of names in NEW_SSA_NAMES and OLD_SSA_NAMES.
      Also register definitions for names whose underlying symbol is
      marked for renaming.  */
+  bool to_delete = false;
   if (register_defs_p (stmt))
     FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, iter, SSA_OP_ALL_DEFS)
-      maybe_register_def (def_p, stmt, gsi);
+      to_delete |= maybe_register_def (def_p, stmt, gsi);
+
+  return to_delete;
 }
 
 
 /* Visit all the successor blocks of BB looking for PHI nodes.  For
    every PHI node found, check if any of its arguments is in
    OLD_SSA_NAMES.  If so, and if the argument has a current reaching
    definition, replace it.  */
 
 static void
 rewrite_update_phi_arguments (basic_block bb)
@@ -2142,22 +2157,25 @@ rewrite_update_dom_walker::before_dom_ch
 	}
 
       if (is_abnormal_phi)
 	SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) = 1;
     }
 
   /* Step 2.  Rewrite every variable used in each statement in the block.  */
   if (bitmap_bit_p (interesting_blocks, bb->index))
     {
       gcc_checking_assert (bitmap_bit_p (blocks_to_update, bb->index));
-      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-        rewrite_update_stmt (gsi_stmt (gsi), gsi);
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
+	if (rewrite_update_stmt (gsi_stmt (gsi), gsi))
+	  gsi_remove (&gsi, true);
+	else
+	  gsi_next (&gsi);
     }
 
   /* Step 3.  Update PHI nodes.  */
   rewrite_update_phi_arguments (bb);
 }
 
 /* Called after visiting block BB.  Unwind BLOCK_DEFS_STACK to restore
    the current reaching definition of every name re-written in BB to
    the original reaching definition before visiting BB.  This
    unwinding must be done in the opposite order to what is done in
Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c	(revision 217007)
+++ gcc/tree-ssa-live.c	(working copy)
@@ -50,20 +50,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
@@ -1096,20 +1097,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
@@ -1432,20 +1437,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: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c	(revision 217007)
+++ gcc/tree-ssa.c	(working copy)
@@ -1181,24 +1181,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
@@ -1208,21 +1209,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;
@@ -1554,32 +1555,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: gcc/tree-ssa.h
===================================================================
--- gcc/tree-ssa.h	(revision 217007)
+++ gcc/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;
 }