Re: update address taken: don't drop clobbers

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

On Thu, Oct 16, 2014 at 4:11 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 16 Oct 2014, Richard Biener wrote:
>
>> On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>> On 10/15/14 08:35, Marc Glisse wrote:
>>>>>
>>>>>
>>>>>
>>>>> Would that extra pass be acceptable?
>>
>>
>> Ugh, rather not.  We have too many passes ;)
>
>
> I expected you would say that...
>
>>>>> 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.
>>>
>>>
>>> I tend to land on the side of minimizing false positives, so the comment
>>> about PR18501 is a "don't care" to me.  If the optimizers remove a dead
>>> assignment and we no longer warn about a potential uninitialized use in
>>> the
>>> dead assignment, then I consider that good.  Not everyone agrees with
>>> that
>>> way of thinking, obviously.
>
>
> I agree with that.
>
>>> So my inclination would be to evaluate independent of the pr18501 issues.
>>> ie, what's the compile-time cost vs runtime benefit of running DCE here.
>>> I'm guessing there's little runtime benefit for this particular case.
>>>
>>> So my next line of thinking would be can we arrange to conditionally run
>>> DCE?  ie, have update_address_taken signal that it did something that has
>>> a
>>> reasonable chance of exposing dead code and only run DCE in that case.
>>> Obviously this only helps if it rarely signals :-)  I don't think we have
>>> any infrastructure for this right now.
>>>
>>> Finally I'd look at how difficult it would be to have
>>> update_address_taken
>>> cleanup after itself.   If the LHS is in SSA form, then if we find it has
>>> no
>>> uses, can we just remove the assignment completely?
>>
>>
>> It doesn't even know that it has no uses (the variable still needs to be
>> written into SSA form).  OTOH it is a missed DSE opportunity before
>> update-address-taken?
>
>
> Uh, we are talking about what happens to a clobber when the variable stops
> having its address taken. We don't want to DSE clobbers (do we?).

No.

> (and va_start is just an example, many transformations could cause this)

Ok, I see.

>> As of premature optimization - into-SSA could notice it created SSA
>> names with no uses and trigger a fast DCE.
>
>
> I am looking into that, it must be doable. It seems not too hard, in
> maybe_register_def, to push all results from make_ssa_name to some
> data-structure (I don't think new_ssa_names gives me that list, but there
> may be other ways to get it without introducing yet another list), and
> either mark them as used in maybe_replace_use or get_reaching_def, or better
> loop through them at the end, checking has_zero_uses (it is a bit wasteful,
> only those coming from clobbers may have 0 uses (or we missed a dce/dse
> earlier), but it should be fast enough, even walking on all ssa_names should
> be fast enough). If we go to that much trouble, we may as well clean them
> while we are there. It isn't obvious to me how to notice unused new
> ssa_names more easily, to trigger a DCE.

Yeah, well.  DCE should in theory also be fast (but it got too many features
so it isn't really fast - maybe guard some stuff in it).

>> Btw, I wonder what this odd folding of variadic builtins is about, and why
>> it is not done in the stdarg pass (and only there), which would be
>> earlier.
>
>
> pass_fold_builtins::execute has:
>
>                 case BUILT_IN_VA_START:
>                 case BUILT_IN_VA_END:
>                 case BUILT_IN_VA_COPY:
>                   /* These shouldn't be folded before pass_stdarg.  */
>                   result = optimize_stdarg_builtin (stmt);
>
> while pass_stdarg only mentions them as:
>
>           /* Don't look at __builtin_va_{start,end}, they are ok.  */
>
> It does seem late, but that will be for someone else to look at.
>
>
> By the way, I noticed something strange in tree-into-ssa.c:
>
> static inline bool
> is_old_name (tree name)
> {
>   unsigned ver = SSA_NAME_VERSION (name);
>   if (!new_ssa_names)
>     return false;
>   return (ver < SBITMAP_SIZE (new_ssa_names)
>           && bitmap_bit_p (old_ssa_names, ver));
> }
>
> There are a lot of "new" in this function about "old".

Eh - indeed.  Patch to fix that pre-approved.

Richard.

>
> --
> Marc Glisse