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

[Chandler Carruth]

On Thu, Feb 6, 2014 at 2:51 AM, Björn Steinbrink <bsteinbr at gmail.com> wrote:

> On 2014.02.05 11:43:07 -0800, Nick Lewycky 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()) {
>
> That's what I did first, but there's a test[1] that expects the cast to
> be omitted when there are no users.
>
> > 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.
>
> Having a direct way to inform CGSCC about the devirtualization would
> also allow to handle the case where the cast is required, which is
> currently not recognized, because the call is RAUW'd with a cast.
>
> > 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 can't judge whether omitting the cast for the result is really
> required or if the cast would get stripped early enough not to cause any
> trouble. If someone could assure me that always emitting the cast here
> is fine, I'll adjust the patch to do that, drop the failing tests and
> make the RAUW unconditional.


There is actually a correct way to trigger ValueHandle updates when we
don't RAUW: ValueHandleBase::ValueIsRAUWd

That should allow a much cleaner implementation of this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140210/031a2805/attachment.html>