<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 6, 2014 at 2:51 AM, Björn Steinbrink <span dir="ltr"><<a href="mailto:bsteinbr@gmail.com" target="_blank">bsteinbr@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On <a href="tel:2014.02.05%2011" value="+12014020511">2014.02.05 11</a>:43:07 -0800, Nick Lewycky wrote:<br>

> Björn Steinbrink wrote:<br>
> >In transformConstExprCastCall, if the replaced call has no users, we<br>
> >currently don't call ReplaceInstUsesWith, which means that ValueHandlers<br>
> >aren't notified about the replacement, but only about the removal of the<br>
> >old call.<br>
> ><br>
> >This means that we can't immediately detect the devirtualization of such<br>
> >calls when updating the call graph in the CGSCC pass, because the old<br>
> >CallSite got invalidated. If some other change then fools the simple<br>
> >counting logic, we fail to re-run the pass on the current function,<br>
> >missing further optimization possibilities opened up by the<br>
> >devirtualized call.<br>
> ><br>
> >As a fix, we should call ReplaceInstUsesWith even if there are no users.<br>
> >The only exception is when the return <span class="">value</span> changed, because we don't<br>
> >add a cast for it when there are no users, and ReplaceInstUsesWith would<br>
> >hit an assertion in that case.<br>
><br>
> The code to <span class="">handle</span> that case is right above it, remove the<br>
> !Caller->use_empty() test on line 1208?<br>
><br>
>    1208   if (OldRetTy != NV->getType() && !Caller->use_empty()) {<br>
<br>
</div>That's what I did first, but there's a test[1] that expects the cast to<br>
be omitted when there are no users.<br>
<div class=""><br>
> Overall I'm torn on this one. I don't like the notion of having<br>
> optimization passes do extra work just to signal things to <span class="">value</span><br>
> <span class="">handle</span> holders. The point of the VH is to not make us need to change<br>
> the optz'ns to follow what's going on. A more explicit way to show<br>
> what's going on would be to call getPassIfAvailable<CallGraph> and<br>
> use its API to keep it up to date.<br>
<br>
</div>Having a direct way to inform CGSCC about the devirtualization would<br>
also allow to <span class="">handle</span> the case where the cast is required, which is<br>
currently not recognized, because the call is RAUW'd with a cast.<br>
<div class=""><br>
> On the other hand that would feel out of place here, and this is an<br>
> important issue to get right. The ad-hoc test that the CGSCC PM does<br>
> wrong anyways, and the notion of holding the CallGraph as "correct"<br>
> while these function passes don't preserve it and modify the call<br>
> graph is dubious at best.<br>
><br>
> I'll accept this approach of doing an extra RAUW to notify the<br>
> callgraph. I'd appreciate if you could make the RAUW itself<br>
> unconditional.<br>
<br>
</div>I can't judge whether omitting the cast for the result is really<br>
required or if the cast would get stripped early enough not to cause any<br>
trouble. If someone could assure me that always emitting the cast here<br>
is fine, I'll adjust the patch to do that, drop the failing tests and<br>
make the RAUW unconditional.</blockquote></div><br>There is actually a correct way to trigger ValueHandle updates when we don't RAUW: ValueHandleBase::ValueIsRAUWd</div><div class="gmail_extra"><br></div><div class="gmail_extra">
That should allow a much cleaner implementation of this.</div></div>