update address taken: don't drop clobbers

[Richard Biener]

On Sun, Jul 6, 2014 at 4:23 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 30 Jun 2014, Jeff Law wrote:
>
>> On 06/28/14 16:33, Marc Glisse wrote:
>>>
>>> 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.
>>
>> And that's what you need to delve deeper into.  Why did it refuse to
>> coalesce?
>>
>> As long as the lifetimes do not overlap, then coalescing should have
>> worked.
>
>
> What is the lifetime of an SSA_NAME with a default definition? The way we
> handle it now, we start from the uses and go back to all blocks that can
> reach one of the uses, since there is no defining statement where we could
> stop (intuitively we should stop where the clobber was, if not earlier).
> This huge lifetime makes it very likely for such an SSA_NAME to conflict
> with others. And if an abnormal phi is involved, and thus we must coalesce,
> there is a problem.
>
> The patch attached (it should probably use ssa_undefined_value_p with a new
> extra argument to say whether we care about partially-undefined) makes the
> lifetime of undefined local variables empty and lets the original patch work
> with:
>       def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
>
> However, I am not convinced reusing the same variable is necessarily the
> best thing. For warnings, we can create a new variable with the same name
> (with .1 added by gcc at the end) and copy the location info (I am not doing
> that yet), so little is lost. A new variable expresses more clearly that the
> value it holds is random crap. If I reuse the same variable, the SRA patch
> doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably
> be changed, but that's starting to be a lot of changes). Creating a new
> variable is also more general. When reading *ssa_name after
> *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to
> reuse so we will need to create a new undefined variable, and if a variable
> is global or a PARM_DECL, its default definition is not an undefined value
> (though it will probably happen in a different pass, so it doesn't have to
> behave the same).
>
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)

Propagators will never replace sth with unefined but they will exploit
the undefinedness.  Constant propagation is even a bit more aggressive.

> Do you have any advice on the right direction?

The patch below would work (btw, please use ssa_undefined_value_p),
but I wonder if it's the conflict code that should get adjustments
rather than the lifeness computation (lifeness of undefined values
shouldn't be needed at all).

Also depends on how we expand an undefined value of course.

Richard.

> --
> Marc Glisse
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 212306)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -1086,20 +1086,28 @@ 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_NAME_IS_DEFAULT_DEF (ssa_name))
> +    {
> +      tree var = SSA_NAME_VAR (ssa_name);
> +      if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
> +       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 +1430,27 @@ 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.  */
> +               tree real_var = SSA_NAME_VAR (var);
> +               if (real_var && TREE_CODE (real_var) == VAR_DECL
> +                   && !is_global_var (real_var))
> +                 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);
>