Re: update address taken: don't drop clobbers

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> 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).

> Some things could be done to improve the error message in uninit:
> - getting the location of the variable,
> - differenciating uninitialized from clobbered,
> but that can come later.
>
> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-unknown-linux-gnu.
>
> 2014-06-30  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR tree-optimization/60770
> gcc/
>         * tree-ssa.c (execute_update_addresses_taken): Don't drop clobbers.
>         * tree-into-ssa.c (maybe_register_def): Replace clobbers with a
>         default definition.
> 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 212109)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -1831,26 +1831,38 @@ 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))

sym should always be a gimple reg here (it's marked for renaming).

> +           {
> +             /* Replace clobber stmts with a default def.  Create a new
> +                variable so we don't later think we must coalesce, which
> would
> +                fail with some ada abnormal PHIs.  Still, we try to keep a
> +                similar name so error messages make sense.  */
> +             unlink_stmt_vdef (stmt);

I think that's redundant with gsi_replace (note that using gsi_replace
looks dangerous here as it calls update_stmt during SSA rewrite...
that might open a can of worms).

> +             gsi_replace (&gsi, gimple_build_nop (), true);
> +             tree id = DECL_NAME (sym);
> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
> +             def = get_or_create_ssa_default_def (cfun, newvar);

So - can't you simply do

    gimple_assign_set_rhs_from_tree (&gsi,
get_or_create_dda_default_def (cfun, sym));

?  Thus replace x = CLOBBER; with x_3 = x_2(D);

> +           }
> +         else

and of course still rewrite the DEF then.  IMHO the copy-propagation
you do is premature optimization.

> +           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.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 212109)
> +++ gcc/tree-ssa.c      (working copy)
> @@ -1607,32 +1607,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;
>