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

[Nick Lewycky]

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.

Nick

> ---
>   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
> +}