Copy Function's calling convention by default when creating a new Call

[serge-sans-paille]

Summary

Altough calling a function whith mismatching Calling Conventions (CC) is defined in the LangRef (as UB), it is a common source of code duplication to set it based on the called function (when it's known), or to forget setting it (which leads to the call being replaced by an undef later in the optimization pipeline).

This commit enforces a default behavior when creating a new CallInst or InvokeInst, as discussed in this thread:

http://lists.llvm.org/pipermail/llvm-dev/2017-November/119121.html

[rnk]

This wasn't what I had in mind. When a frontend (or any other IR generator) creates a call, it should have a source of type information telling it the function prototype and the calling convention (in clang, the AST). It should also apply any other wacky calling convention flags like -mregparm and other things. In the special case where the frontend is emitting a call to a function defined in the current TU, we should have a check that the conventions match. I was imagining that you'd make IRBuilder implement that assert, and then add an alternative CreateCallWithConvention entry point or something like that to allow the frontend to override the check.

[serge-sans-paille]

In D40249#930728, @rnk wrote:

This wasn't what I had in mind. When a frontend (or any other IR generator) creates a call, it should have a source of type information telling it the function prototype and the calling convention (in clang, the AST). It should also apply any other wacky calling convention flags like -mregparm and other things.

I totally agree. This means extending the current `Create with a CallingConv::ID` parameter to be able to express that in the builder. Note that my implementation goes in the same spirit (if no information, use the function's CallingConvention)

In the special case where the frontend is emitting a call to a function defined in the current TU, we should have a check that the conventions match. I was imagining that you'd make IRBuilder implement that assert, and then add an alternative CreateCallWithConvention entry point or something like that to allow the frontend to override the check.

I'm not sure that's the best approach. With the approach described above, either the user does not specify the callingconv and it's always consistent (if the function is statically known, that is), or it forces it and we have to trust him/her. Asserting while the LangRef allows mismatching CallingConv seems strange to me. Helping the user to avoid forgeting to set the calling conv looks safer.

[serge-sans-paille]

@rnk: I've implemented the extra CallingConvention argument in the various create statements, but I'm very unhappy with the result: it requires a lot of extra create overload, the number of optional arguments grows and the overall gain within the actual codebase is small. I honestly think my current proposal, even if modest, is already a good step forward.

As an argument, consider the fact that this patch removes 41 lines of mostly redundant code.

[serge-sans-paille]

@rnk gentle up :-)

[serge-sans-paille]

@rnk weekly bump :-)

[rnk]

I'd feel better about this if there was some other helper we used to set inreg and other ABI attributes on these calls.

[serge-sans-paille]

restoring previous diff, I accidentally uploaded an incorrect one on that review :-/

lib/CodeGen/StackProtector.cpp

       CallInst *Call = B.CreateCall(GuardCheck, {Guard});
       llvm::Function *Function = cast<llvm::Function>(GuardCheck);
       Call->setAttributes(Function->getAttributes());
-      Call->setCallingConv(Function->getCallingConv());
     } else {
       // Generate the epilogue with inline instrumentation.
       // If we do not support SelectionDAG based tail calls, generate IR level

lib/IR/Instructions.cpp

 //                        CallInst Implementation
 //===----------------------------------------------------------------------===//
 
+template <class CallTy>
+static void tryCopyCallingConvention(CallTy *To, Value const *From) {
+  // ensure Calling Convention consistency if possible
+  if (Function const *F = dyn_cast<Function const>(From->stripPointerCasts()))
+    To->setCallingConv(F->getCallingConv());
+}
+
 void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args,
                     ArrayRef<OperandBundleDef> Bundles, const Twine &NameStr) {
   this->FTy = FTy;

   assert(It + 1 == op_end() && "Should add up!");
 
   setName(NameStr);
+  tryCopyCallingConvention(this, Func);
 }
 
 void CallInst::init(Value *Func, const Twine &NameStr) {

   assert(FTy->getNumParams() == 0 && "Calling a function with bad signature");
 
   setName(NameStr);
+  tryCopyCallingConvention(this, Func);
 }
 
 CallInst::CallInst(Value *Func, const Twine &Name,

     Result = CallInst::Create(FreeFunc, PtrCast, Bundles, "");
   }
   Result->setTailCall();
-  if (Function *F = dyn_cast<Function>(FreeFunc))
-    Result->setCallingConv(F->getCallingConv());
-
   return Result;
 }
 

   assert(It + 3 == op_end() && "Should add up!");
 
   setName(NameStr);
+  tryCopyCallingConvention(this, Fn);
 }
 
 InvokeInst::InvokeInst(const InvokeInst &II)

lib/Target/AMDGPU/AMDGPULibCalls.cpp

 CallInst *CreateCallEx(IRB &B, Value *Callee, Value *Arg, const Twine &Name="")
 {
   CallInst *R = B.CreateCall(Callee, Arg, Name);
-  if (Function* F = dyn_cast<Function>(Callee))
-    R->setCallingConv(F->getCallingConv());
   return R;
 }
 

 CallInst *CreateCallEx2(IRB &B, Value *Callee, Value *Arg1, Value *Arg2,
                         const Twine &Name="") {
   CallInst *R = B.CreateCall(Callee, {Arg1, Arg2}, Name);
-  if (Function* F = dyn_cast<Function>(Callee))
-    R->setCallingConv(F->getCallingConv());
   return R;
 }
 

lib/Transforms/Utils/BuildLibCalls.cpp

                                             B.getInt8PtrTy());
   inferLibFuncAttributes(*M->getFunction("strlen"), *TLI);
   CallInst *CI = B.CreateCall(StrLen, castToCStr(Ptr, B), "strlen");
-  if (const Function *F = dyn_cast<Function>(StrLen->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

   inferLibFuncAttributes(*M->getFunction("strchr"), *TLI);
   CallInst *CI = B.CreateCall(
       StrChr, {castToCStr(Ptr, B), ConstantInt::get(I32Ty, C)}, "strchr");
-  if (const Function *F = dyn_cast<Function>(StrChr->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
   return CI;
 }
 

   inferLibFuncAttributes(*M->getFunction("strncmp"), *TLI);
   CallInst *CI = B.CreateCall(
       StrNCmp, {castToCStr(Ptr1, B), castToCStr(Ptr2, B), Len}, "strncmp");
-
-  if (const Function *F = dyn_cast<Function>(StrNCmp->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

   inferLibFuncAttributes(*M->getFunction(Name), *TLI);
   CallInst *CI =
       B.CreateCall(StrCpy, {castToCStr(Dst, B), castToCStr(Src, B)}, Name);
-  if (const Function *F = dyn_cast<Function>(StrCpy->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
   return CI;
 }
 

   inferLibFuncAttributes(*M->getFunction(Name), *TLI);
   CallInst *CI = B.CreateCall(
       StrNCpy, {castToCStr(Dst, B), castToCStr(Src, B), Len}, "strncpy");
-  if (const Function *F = dyn_cast<Function>(StrNCpy->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
   return CI;
 }
 

   Dst = castToCStr(Dst, B);
   Src = castToCStr(Src, B);
   CallInst *CI = B.CreateCall(MemCpy, {Dst, Src, Len, ObjSize});
-  if (const Function *F = dyn_cast<Function>(MemCpy->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
   return CI;
 }
 

                                          DL.getIntPtrType(Context));
   inferLibFuncAttributes(*M->getFunction("memchr"), *TLI);
   CallInst *CI = B.CreateCall(MemChr, {castToCStr(Ptr, B), Val, Len}, "memchr");
-
-  if (const Function *F = dyn_cast<Function>(MemChr->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

   inferLibFuncAttributes(*M->getFunction("memcmp"), *TLI);
   CallInst *CI = B.CreateCall(
       MemCmp, {castToCStr(Ptr1, B), castToCStr(Ptr2, B), Len}, "memcmp");
-
-  if (const Function *F = dyn_cast<Function>(MemCmp->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

   CI->setAttributes(Attrs.removeAttribute(B.getContext(),
                                           AttributeList::FunctionIndex,
                                           Attribute::Speculatable));
-  if (const Function *F = dyn_cast<Function>(Callee->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

                                          Op2->getType());
   CallInst *CI = B.CreateCall(Callee, {Op1, Op2}, Name);
   CI->setAttributes(Attrs);
-  if (const Function *F = dyn_cast<Function>(Callee->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

                               /*isSigned*/true,
                               "chari"),
                               "putchar");
-
-  if (const Function *F = dyn_cast<Function>(PutChar->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
   return CI;
 }
 

       M->getOrInsertFunction("puts", B.getInt32Ty(), B.getInt8PtrTy());
   inferLibFuncAttributes(*M->getFunction("puts"), *TLI);
   CallInst *CI = B.CreateCall(PutS, castToCStr(Str, B), "puts");
-  if (const Function *F = dyn_cast<Function>(PutS->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
   return CI;
 }
 

   Char = B.CreateIntCast(Char, B.getInt32Ty(), /*isSigned*/true,
                          "chari");
   CallInst *CI = B.CreateCall(F, {Char, File}, "fputc");
-
-  if (const Function *Fn = dyn_cast<Function>(F->stripPointerCasts()))
-    CI->setCallingConv(Fn->getCallingConv());
   return CI;
 }
 

   if (File->getType()->isPointerTy())
     inferLibFuncAttributes(*M->getFunction(FPutsName), *TLI);
   CallInst *CI = B.CreateCall(F, {castToCStr(Str, B), File}, "fputs");
-
-  if (const Function *Fn = dyn_cast<Function>(F->stripPointerCasts()))
-    CI->setCallingConv(Fn->getCallingConv());
   return CI;
 }
 

   CallInst *CI =
       B.CreateCall(F, {castToCStr(Ptr, B), Size,
                        ConstantInt::get(DL.getIntPtrType(Context), 1), File});
-
-  if (const Function *Fn = dyn_cast<Function>(F->stripPointerCasts()))
-    CI->setCallingConv(Fn->getCallingConv());
   return CI;
 }

lib/Transforms/Utils/SimplifyLibCalls.cpp

                                          PtrType, PtrType);
   CallInst *CI = B.CreateCall(Calloc, { Num, Size }, "calloc");
 
-  if (const auto *F = dyn_cast<Function>(Calloc->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
   return CI;
 }
 

           M->getOrInsertFunction(TLI->getName(LdExp), Op->getType(),
                                  Op->getType(), B.getInt32Ty());
       CallInst *CI = B.CreateCall(NewCallee, {One, LdExpArg});
-      if (const Function *F = dyn_cast<Function>(Callee->stripPointerCasts()))
-        CI->setCallingConv(F->getCallingConv());
 
       return CI;
     }