[InstCombine] Remove unneeded VarArg casts.

[fhahn]

Summary

This combine was suggested in D41335.

[rnk]

@rjmccall, does this cause problems on Darwin? I seem to recall that there are some ABIs (AArch64 Darwin) where arguments in the varargs pack are passed differently from the ones that are prototyped.

If that is a problem, do you think we can permit this simplification if we see a non-interposable definition of the callee? In this case, instcombine should "know" the true prototype of the callee and should be adjusting things so the types match.

[efriedma]

There are many ABIs with special cases for varargs. The x86-64 Linux ABI specifically defines that vararg calls need to initialize AL, and other calls don't. The hard-float ABI on ARM Linux usually pass "double" arguments in floating-point regsiters, but varargs functions pass them in integer registers. Other ABIs might do more exotic things I'm not thinking of.

If you can prove the callee is in fact not a varargs function, you might be able to do something like this, but the type of a function declaration isn't enough to prove that.

[rnk]

In D41633#965959, @efriedma wrote:

There are many ABIs with special cases for varargs. The x86-64 Linux ABI specifically defines that vararg calls need to initialize AL, and other calls don't. The hard-float ABI on ARM Linux usually pass "double" arguments in floating-point regsiters, but varargs functions pass them in integer registers. Other ABIs might do more exotic things I'm not thinking of.

If you can prove the callee is in fact not a varargs function, you might be able to do something like this, but the type of a function declaration isn't enough to prove that.

Makes sense. Fortunately, we can prove the callee is not varargs in exactly the circumstances that matter: when we would be able to inline, i.e. we have a non-interposable definition.

[rjmccall]

I think at least one of the MIPS ABIs does the same thing with varargs as AArch64 on Darwin.

Eli is right, the type of a function declaration is totally irrelevant under current IR semantics. The only thing that contributes to the CC of a call is the combination of (1) the type of the callee value, (2) the named calling convention, and (3) the attributes on the call site. I can declare malloc as taking three floats and returning void, and as long as I cast it to i8* (i64)* at every call site, everything should work. If you can prove that you have the dynamic definition of the callee, you can apply UB rules, but you don't have that here — you don't have a definition at all.

[fhahn]

Happy new year and thank you very much for having a look!

If ext_method is a definition InstCombine already gets rid of the bitcast in the test case. It sounds like this is the best we can do. I'll abandon this review. But I think it would be worth to commit the test case (with an definition of ext_method instead of declare ) to the file, as this case seems not to be covered. Unless there are any objection, I'll do that as a NFC.

lib/Transforms/InstCombine/InstCombineCalls.cpp

if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
           II->getOperand(0) == TrampMem)
         return II;
     if (Inst->mayWriteToMemory())
       return nullptr;
   }
   return nullptr;
 }
 
+// Try to convert VarArg calls to regular calls if the callee is a regular
+// function, but is casted to a VarArg function, in case the actual arguments
+// match the callee's signature.
+static bool transformVarArgCast(CallSite CS) {
+  auto *Callee = dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
+  if (CS.getCalledValue() == Callee || !Callee)
+    return false;
+
+  FunctionType *CalleeTy = Callee->getFunctionType();
+  if (CalleeTy->isVarArg() ||
+      CalleeTy->getNumParams() != CS.getNumArgOperands() ||
+      CalleeTy->getReturnType() != CS.getFunctionType()->getReturnType())
+    return false;
+
+  // Check if operand types at the call site match the parameter types
+  // of the original function.
+  for (unsigned i = 0; i < CS.getNumArgOperands(); i++)
+    if (CS.getArgOperand(i)->getType() !=
+        Callee->getFunctionType()->getParamType(i))
+      return false;
+
+  CS.mutateFunctionType(Callee->getFunctionType());
+  CS.setCalledFunction(Callee);
+  return true;
+}
+
 // Given a call to llvm.adjust.trampoline, find and return the corresponding
 // call to llvm.init.trampoline if the call to the trampoline can be optimized
 // to a direct call to a function.  Otherwise return NULL.
 static IntrinsicInst *findInitTrampoline(Value *Callee) {
   Callee = Callee->stripPointerCasts();
   IntrinsicInst *AdjustTramp = dyn_cast<IntrinsicInst>(Callee);
   if (!AdjustTramp ||
       AdjustTramp->getIntrinsicID() != Intrinsic::adjust_trampoline)
Instruction *InstCombiner::visitCallSite(CallSite CS) {
   // delete the instruction now.
   if (CallInst *CI = dyn_cast<CallInst>(CS.getInstruction())) {
     Instruction *I = tryOptimizeCall(CI);
     // If we changed something return the result, etc. Otherwise let
     // the fallthrough check.
     if (I) return eraseInstFromFunction(*I);
   }
 
+  transformVarArgCast(CS);
   return Changed ? CS.getInstruction() : nullptr;
 }
 
 /// 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)

test/Transforms/InstCombine/call.ll

 define i32 @test17() {
 entry:
   %C = call i32 @pr28655(i32 0)
   ret i32 %C
 }
 ; CHECK-LABEL: @test17(
 ; CHECK: call i32 @pr28655(i32 0)
 ; CHECK: ret i32 0
+
+declare void @ext_method(i8*, i32) local_unnamed_addr
+
+define void @test_cast_to_vararg(i8* %this) {
+; CHECK-LABEL: test_cast_to_vararg
+; CHECK:  call void @ext_method(i8* %this, i32 42)
+  call void (i8*, ...) bitcast (void (i8*, i32)* @ext_method to void (i8*, ...)*)(i8* %this, i32 42)
+  ret void
+}