[PATCH] Fix detection of devirtualized calls that have no users

[Chandler Carruth]

On Wed, Feb 5, 2014 at 11:43 AM, Nick Lewycky <nicholas at mxc.ca> wrote:

> Björn Steinbrink wrote:
>
>> In transformConstExprCastCall, if the replaced call has no users, we
>> currently don't call ReplaceInstUsesWith, which means that ValueHandlers
>> aren't notified about the replacement, but only about the removal of the
>> old call.
>>
>> This means that we can't immediately detect the devirtualization of such
>> calls when updating the call graph in the CGSCC pass, because the old
>> CallSite got invalidated. If some other change then fools the simple
>> counting logic, we fail to re-run the pass on the current function,
>> missing further optimization possibilities opened up by the
>> devirtualized call.
>>
>> As a fix, we should call ReplaceInstUsesWith even if there are no users.
>> The only exception is when the return value changed, because we don't
>> add a cast for it when there are no users, and ReplaceInstUsesWith would
>> hit an assertion in that case.
>>
>
> The code to handle that case is right above it, remove the
> !Caller->use_empty() test on line 1208?
>
>    1208   if (OldRetTy != NV->getType() && !Caller->use_empty()) {
>
> Overall I'm torn on this one. I don't like the notion of having
> optimization passes do extra work just to signal things to value handle
> holders. The point of the VH is to not make us need to change the optz'ns
> to follow what's going on. A more explicit way to show what's going on
> would be to call getPassIfAvailable<CallGraph> and use its API to keep it
> up to date.
>
> On the other hand that would feel out of place here, and this is an
> important issue to get right. The ad-hoc test that the CGSCC PM does wrong
> anyways, and the notion of holding the CallGraph as "correct" while these
> function passes don't preserve it and modify the call graph is dubious at
> best.
>
> I'll accept this approach of doing an extra RAUW to notify the callgraph.
> I'd appreciate if you could make the RAUW itself unconditional.


I share your feelings Nick. For my part (having studdied this recently in
the process of working on a replacement for the new pass manager) I think
the correct thing is to form a cast and *always* RAUW. As much as I dislike
using RAUW as a magical signal to ValueHandles, I dislike inconsistently
iterating on the function to discover further simplifications much, much
more.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140206/0bfcba48/attachment.html>