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

[Björn Steinbrink]

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.

Thanks,
Björn

[1] Transforms/InstCombine/2008-01-06-BitCastAttributes.ll

> >---
> >  lib/Transforms/InstCombine/InstCombineCalls.cpp  |    4 +++-
> >  test/Transforms/InstCombine/cast-call-combine.ll |   22 ++++++++++++++++++++++
> >  2 files changed, 25 insertions(+), 1 deletions(-)
> >  create mode 100644 test/Transforms/InstCombine/cast-call-combine.ll
> >
> >diff --git a/lib/Transforms/InstCombine/InstCombineCalls.cpp b/lib/Transforms/InstCombine/InstCombineCalls.cpp
> >index 8e308ec..b72851d 100644
> >--- a/lib/Transforms/InstCombine/InstCombineCalls.cpp
> >+++ b/lib/Transforms/InstCombine/InstCombineCalls.cpp
> >@@ -1225,7 +1225,9 @@ bool InstCombiner::transformConstExprCastCall(CallSite CS) {
> >      }
> >    }
> >
> >-  if (!Caller->use_empty())
> >+  // ReplaceInstUsesWith even when there are no users as long as types match.
> >+  // This allows ValueHandlers and custom metadata to adjust itself.
> >+  if (OldRetTy == NV->getType())
> >      ReplaceInstUsesWith(*Caller, NV);
> >
> >    EraseInstFromFunction(*Caller);
> >diff --git a/test/Transforms/InstCombine/cast-call-combine.ll b/test/Transforms/InstCombine/cast-call-combine.ll
> >new file mode 100644
> >index 0000000..4eb2478
> >--- /dev/null
> >+++ b/test/Transforms/InstCombine/cast-call-combine.ll
> >@@ -0,0 +1,22 @@
> >+; RUN: opt<  %s -always-inline -instcombine -S | FileCheck %s
> >+
> >+define internal void @foo(i16*) alwaysinline {
> >+  ret void
> >+}
> >+
> >+define void @bar() noinline noreturn {
> >+  unreachable
> >+}
> >+
> >+define void @test() {
> >+  br i1 false, label %then, label %else
> >+
> >+then:
> >+  call void @bar()
> >+  unreachable
> >+
> >+else:
> >+  ; CHECK-NOT: call
> >+  call void bitcast (void (i16*)* @foo to void (i8*)*) (i8* null)
> >+  ret void
> >+}
>