[InstCombine] Don't strip function type casts from musttail calls

[rnk]

Summary

The cast simplifications that instcombine does here do not make any
attempt to obey the verifier rules for musttail calls. Therefore we have
to disable them.

[efriedma]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

4011 ↗

(On Diff #140687)

Why does it matter whether the function is a thunk? It seems like this would be a problem for any musttail call.

[rnk]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

4011 ↗

(On Diff #140687)

That's true. I can imagine LTO situations where this would go wrong without any variadic argument pack forwarding being involved.

[rnk]

• this isn't about thunks

[rnk]

• rebase over committed changes

[efriedma]

LGTM with one minor change.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

4012 ↗

(On Diff #140699)

CS.isMustTailCall().

[rnk]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

4012 ↗

(On Diff #140699)

Thanks!

llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp

 
 /// If the callee is a constexpr cast of a function, attempt to move the cast to
 /// the arguments of the call/invoke.
 bool InstCombiner::transformConstExprCastCall(CallSite CS) {
   auto *Callee = dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
   if (!Callee)
     return false;
 
-  // The prototype of a thunk is a lie. Don't directly call such a function.
+  // If this is a call to a thunk function, don't remove the cast. Thunks are
+  // used to transparently forward all incoming parameters and outgoing return
+  // values, so it's important to leave the cast in place.
   if (Callee->hasFnAttribute("thunk"))
     return false;
 
+  // If this is a musttail call, the callee's prototype must match the caller's
+  // prototype with the exception of pointee types. The code below doesn't
+  // implement that, so we can't do this transform.
+  // TODO: Do the transform if it only requires adding pointer casts.
+  if (CS.isMustTailCall())
+    return false;
+
   Instruction *Caller = CS.getInstruction();
   const AttributeList &CallerPAL = CS.getAttributes();
 
   // Okay, this is a cast from a function to a different type.  Unless doing so
   // would cause a type conversion of one of our arguments, change this call to
   // be a direct call with arguments casted to the appropriate types.
   FunctionType *FT = Callee->getFunctionType();
   Type *OldRetTy = Caller->getType();

llvm/trunk/test/Transforms/InstCombine/musttail-thunk.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+; These are both direct calls, but make sure instcombine leaves the casts
+; alone.
+
+define i32 @call_thunk(i32 %x, i32 %y) {
+  %r = call i32 bitcast (void (i32, ...)* @inc_first_arg_thunk to i32 (i32, i32)*)(i32 %x, i32 %y)
+  ret i32 %r
+}
+
+; CHECK-LABEL: define i32 @call_thunk(i32 %x, i32 %y)
+; CHECK:   %r = call i32 bitcast (void (i32, ...)* @inc_first_arg_thunk to i32 (i32, i32)*)(i32 %x, i32 %y)
+; CHECK:   ret i32 %r
+
+define internal void @inc_first_arg_thunk(i32 %arg1, ...) #0 {
+entry:
+  %inc = add i32 %arg1, 1
+  musttail call void (i32, ...) bitcast (i32 (i32, i32)* @plus to void (i32, ...)*)(i32 %inc, ...)
+  ret void
+}
+
+; CHECK-LABEL: define internal void @inc_first_arg_thunk(i32 %arg1, ...) #0
+; CHECK:   %inc = add i32 %arg1, 1
+; CHECK:   musttail call void (i32, ...) bitcast (i32 (i32, i32)* @plus to void (i32, ...)*)(i32 %inc, ...)
+; CHECK:   ret void
+
+define internal i32 @plus(i32 %x, i32 %y) {
+  %r = add i32 %x, %y
+  ret i32 %r
+}
+
+attributes #0 = { "thunk" }