Re: update address taken: don't drop clobbers

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

On Sun, 7 Sep 2014, Marc Glisse wrote:

> On Sun, 27 Jul 2014, Marc Glisse wrote:
>
> > 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)
>
> I would like some guidance on this. I just tried this trivial patch:
>
>       NEXT_PASS (pass_split_crit_edges);
> +      NEXT_PASS (pass_dce);
>       NEXT_PASS (pass_late_warn_uninitialized);
>
> and it does not cause any regression, it even XPASS gfortran.dg/reassoc_6.f 
> for some reason. The FIXME note just above in passes.def mentions 2 testcases 
> that are already xfailed anyway.
>
> Would that extra pass be acceptable?
>
> Otherwise, what do you think should be responsible for cleaning up the dead 
> assignments?

Does anyone have an opinion on which side needs to be improved? As a 
reminder:

- we have a va_list with its address taken by va_start/va_end.
- fab lowers va_start/va_end and the list doesn't have its address taken 
anymore.
- update_address_taken replaces the clobber: list =v {}; with an 
assignment of an undefined value: list_6 = list_2(D);
- uninit warns about this.

Some possible directions:
- "prematurely" optimize in update_address_taken so we don't generate the 
useless assignment.
- add a dce pass before uninit.

--
Marc Glisse