update address taken: don't drop clobbers

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.

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.

> +           {
> +             /* 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.

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

Otherwise the patch looks ok.

Thanks for your patience.
Richard.

> +           }
> +         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: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 216689)
> +++ gcc/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: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 216689)
> +++ gcc/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: gcc/tree-ssa.h
> ===================================================================
> --- gcc/tree-ssa.h      (revision 216689)
> +++ 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;
>  }
>
>

On 10/25/14 10:29, Marc Glisse 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.
A clobber doesn't generate any code, it's really best to think of it as 
a marker.  It doesn't throw, shouldn't have histrograms or anything else 
of importance.

Approved.

Jeff