Re: update address taken: don't drop clobbers

[Marc Glisse <marc.glisse@inria.fr>]

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?

-- 
Marc Glisse