Re: update address taken: don't drop clobbers

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

On Thu, 10 Jul 2014, Richard Biener wrote:

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

Using your version, I end up with spurious warnings, in particular for 
va_list. pass_fold_builtins stops va_start/va_end taking the address of 
the list, so we get:

  list_6 = list_2(D);

in place of the clobber at the end of the function. And there is no 
DCE-like pass afterwards, so we warn for the use of list_2(D).
(passes.def contains a comment about running dce before uninit)

I don't know if update_address_taken could avoid generating this 
assignment where the lhs has 0 use, but this shows the optimization is not 
completely premature.

(uninit could also check for this case, but that feels like a bad hack)

--
Marc Glisse