[InstCombine] when calling conventions are compatible, don't convert the call to undef idiom

[ychen]

Summary

D24453 enabled libcalls simplication for ARM PCS. This may cause
caller/callee calling conventions mismatch in some situations such as
LTO. This patch makes instcombine aware that the compatible calling
conventions differences are benign (not emitting undef idom).

[ychen]

• fix styling issue

[ychen]

ping..

[ychen]

• fix typo

[lebedev.ri]

LGTM, since we were already doing that in SimplifyLibCalls.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

2121–2122

[ychen]

• address comments

[nikic]

I've temporarily reverted this change because it causes a significant compile-time regression, see https://llvm-compile-time-tracker.com/compare.php?from=4b7bad9eaea2233521a94f6b096aaa88dc584e23&to=f4d682d6ce6c5b3a41a0acf297507c82f5c21eef&stat=instructions. I assume that wasn't intentional.

Looking at the code, what stands out to me is that you're now parsing the target triple eagerly, even though it is only needed if AAPCS is actually involved. Passing it into the function as StringRef instead of Triple is probably sufficient. (More generally, it might make sense to store a parsed target triple in the module...)

[ychen]

In D99773#2684148, @nikic wrote:

I've temporarily reverted this change because it causes a significant compile-time regression, see https://llvm-compile-time-tracker.com/compare.php?from=4b7bad9eaea2233521a94f6b096aaa88dc584e23&to=f4d682d6ce6c5b3a41a0acf297507c82f5c21eef&stat=instructions. I assume that wasn't intentional.

Looking at the code, what stands out to me is that you're now parsing the target triple eagerly, even though it is only needed if AAPCS is actually involved. Passing it into the function as StringRef instead of Triple is probably sufficient. (More generally, it might make sense to store a parsed target triple in the module...)

Thanks for the report. Passing Stringref does put it back to normal.
https://llvm-compile-time-tracker.com/compare.php?from=23b8264b5255efdc7c87c189feab04520a1979d5&to=69359ebeee6e3bd0e786a51ca542762d3aa4e606&stat=instructions

llvm/include/llvm/Analysis/TargetLibraryInfo.h

public:
   /// Returns the size of the wchar_t type in bytes or 0 if the size is unknown.
   /// This queries the 'wchar_size' metadata.
   unsigned getWCharSize(const Module &M) const;
 
   /// Returns the largest vectorization factor used in the list of
   /// vector functions.
   void getWidestVF(StringRef ScalarF, ElementCount &FixedVF,
                    ElementCount &Scalable) const;
+
+  /// Returns true if call site / callee has cdecl-compatible calling
+  /// conventions.
+  static bool isCallingConvCCompatible(CallBase *CI);
+  static bool isCallingConvCCompatible(Function *Callee);
 };
 
 /// Provides information about what library functions are available for
 /// the current target.
 ///
 /// This both allows optimizations to handle them specially and frontends to
 /// disable such optimizations through -fno-builtin etc.
 class TargetLibraryInfo {

llvm/lib/Analysis/TargetLibraryInfo.cpp

static bool hasBcmp(const Triple &TT) {
   // implementations of the libc still have it.
   if (TT.isOSLinux())
     return TT.isGNUEnvironment() || TT.isMusl();
   // Both NetBSD and OpenBSD are planning to remove the function. Windows does
   // not have it.
   return TT.isOSFreeBSD() || TT.isOSSolaris();
 }
 
+static bool isCallingConvCCompatible(CallingConv::ID CC, Triple TT,
+                                     FunctionType *FuncTy) {
+  switch (CC) {
+  default:
+    return false;
+  case llvm::CallingConv::C:
+    return true;
+  case llvm::CallingConv::ARM_APCS:
+  case llvm::CallingConv::ARM_AAPCS:
+  case llvm::CallingConv::ARM_AAPCS_VFP: {
+
+    // The iOS ABI diverges from the standard in some cases, so for now don't
+    // try to simplify those calls.
+    if (TT.isiOS())
+      return false;
+
+    if (!FuncTy->getReturnType()->isPointerTy() &&
+        !FuncTy->getReturnType()->isIntegerTy() &&
+        !FuncTy->getReturnType()->isVoidTy())
+      return false;
+
+    for (auto *Param : FuncTy->params()) {
+      if (!Param->isPointerTy() && !Param->isIntegerTy())
+        return false;
+    }
+    return true;
+  }
+  }
+  return false;
+}
+
+bool TargetLibraryInfoImpl::isCallingConvCCompatible(CallBase *CI) {
+  return ::isCallingConvCCompatible(CI->getCallingConv(),
+                                    Triple(CI->getModule()->getTargetTriple()),
+                                    CI->getFunctionType());
+}
+
+bool TargetLibraryInfoImpl::isCallingConvCCompatible(Function *F) {
+  return ::isCallingConvCCompatible(F->getCallingConv(),
+                                    Triple(F->getParent()->getTargetTriple()),
+                                    F->getFunctionType());
+}
+
 /// Initialize the set of available library functions based on the specified
 /// target triple. This should be carefully written so that a missing target
 /// triple gets a sane set of defaults.
 static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
                        ArrayRef<StringLiteral> StandardNames) {
   // Verify that the StandardNames array is in alphabetical order.
   assert(
       llvm::is_sorted(StandardNames,

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

if (Function *CalleeF = dyn_cast<Function>(Callee)) {
     if (Call.isConvergent() && !CalleeF->isConvergent() &&
         !CalleeF->isIntrinsic()) {
       LLVM_DEBUG(dbgs() << "Removing convergent attr from instr " << Call
                         << "\n");
       Call.setNotConvergent();
       return &Call;
     }
 
     // If the call and callee calling conventions don't match, this call must
     // be unreachable, as the call is undefined.
-    if (CalleeF->getCallingConv() != Call.getCallingConv() &&
+    if ((CalleeF->getCallingConv() != Call.getCallingConv() &&
+         !(CalleeF->getCallingConv() == llvm::CallingConv::C &&
+           TargetLibraryInfoImpl::isCallingConvCCompatible(&Call)) &&
+         !(Call.getCallingConv() == llvm::CallingConv::C &&
+           TargetLibraryInfoImpl::isCallingConvCCompatible(CalleeF))) &&
         // Only do this for calls to a function with a body.  A prototype may
         // not actually end up matching the implementation's calling conv for a
         // variety of reasons (e.g. it may be written in assembly).
         !CalleeF->isDeclaration()) {
       Instruction *OldCall = &Call;
       CreateNonTerminatorUnreachable(OldCall);
       // If OldCall does not return void then replaceInstUsesWith undef.
       // This allows ValueHandlers and custom metadata to adjust itself.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

 // Helper Functions
 //===----------------------------------------------------------------------===//
 
 static bool ignoreCallingConv(LibFunc Func) {
   return Func == LibFunc_abs || Func == LibFunc_labs ||
          Func == LibFunc_llabs || Func == LibFunc_strlen;
 }
 
-static bool isCallingConvCCompatible(CallInst *CI) {
-  switch(CI->getCallingConv()) {
-  default:
-    return false;
-  case llvm::CallingConv::C:
-    return true;
-  case llvm::CallingConv::ARM_APCS:
-  case llvm::CallingConv::ARM_AAPCS:
-  case llvm::CallingConv::ARM_AAPCS_VFP: {
-
-    // The iOS ABI diverges from the standard in some cases, so for now don't
-    // try to simplify those calls.
-    if (Triple(CI->getModule()->getTargetTriple()).isiOS())
-      return false;
-
-    auto *FuncTy = CI->getFunctionType();
-
-    if (!FuncTy->getReturnType()->isPointerTy() &&
-        !FuncTy->getReturnType()->isIntegerTy() &&
-        !FuncTy->getReturnType()->isVoidTy())
-      return false;
-
-    for (auto Param : FuncTy->params()) {
-      if (!Param->isPointerTy() && !Param->isIntegerTy())
-        return false;
-    }
-    return true;
-  }
-  }
-  return false;
-}
-
 /// Return true if it is only used in equality comparisons with With.
 static bool isOnlyUsedInEqualityComparison(Value *V, Value *With) {
   for (User *U : V->users()) {
     if (ICmpInst *IC = dyn_cast<ICmpInst>(U))
       if (IC->isEquality() && IC->getOperand(1) == With)
         continue;
     // Unknown instruction.
     return false;

 
 Value *LibCallSimplifier::optimizeStringMemoryLibCall(CallInst *CI,
                                                       IRBuilderBase &Builder) {
   LibFunc Func;
   Function *Callee = CI->getCalledFunction();
   // Check for string/memory library functions.
   if (TLI->getLibFunc(*Callee, Func) && TLI->has(Func)) {
     // Make sure we never change the calling convention.
-    assert((ignoreCallingConv(Func) ||
-            isCallingConvCCompatible(CI)) &&
+    assert(
+        (ignoreCallingConv(Func) ||
+         TargetLibraryInfoImpl::isCallingConvCCompatible(CI)) &&
         "Optimizing string/memory libcall would change the calling convention");
     switch (Func) {
     case LibFunc_strcat:
       return optimizeStrCat(CI, Builder);
     case LibFunc_strncat:
       return optimizeStrNCat(CI, Builder);
     case LibFunc_strchr:
       return optimizeStrChr(CI, Builder);
     case LibFunc_strrchr:
Value *LibCallSimplifier::optimizeCall(CallInst *CI, IRBuilderBase &Builder) {
   // TODO: Split out the code below that operates on FP calls so that
   //       we can all non-FP calls with the StrictFP attribute to be
   //       optimized.
   if (CI->isNoBuiltin())
     return nullptr;
 
   LibFunc Func;
   Function *Callee = CI->getCalledFunction();
-  bool isCallingConvC = isCallingConvCCompatible(CI);
+  bool IsCallingConvC = TargetLibraryInfoImpl::isCallingConvCCompatible(CI);
 
   SmallVector<OperandBundleDef, 2> OpBundles;
   CI->getOperandBundlesAsDefs(OpBundles);
 
   IRBuilderBase::OperandBundlesGuard Guard(Builder);
   Builder.setDefaultOperandBundles(OpBundles);
 
   // Command-line parameter overrides instruction attribute.
   // This can't be moved to optimizeFloatingPointLibCall() because it may be
   // used by the intrinsic optimizations.
   if (EnableUnsafeFPShrink.getNumOccurrences() > 0)
     UnsafeFPShrink = EnableUnsafeFPShrink;
   else if (isa<FPMathOperator>(CI) && CI->isFast())
     UnsafeFPShrink = true;
 
   // First, check for intrinsics.
   if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CI)) {
-    if (!isCallingConvC)
+    if (!IsCallingConvC)
       return nullptr;
     // The FP intrinsics have corresponding constrained versions so we don't
     // need to check for the StrictFP attribute here.
     switch (II->getIntrinsicID()) {
     case Intrinsic::pow:
       return optimizePow(CI, Builder);
     case Intrinsic::exp2:
       return optimizeExp2(CI, Builder);
if (SimplifiedCI && SimplifiedCI->getCalledFunction()) {
       }
     }
     return SimplifiedFortifiedCI;
   }
 
   // Then check for known library functions.
   if (TLI->getLibFunc(*Callee, Func) && TLI->has(Func)) {
     // We never change the calling convention.
-    if (!ignoreCallingConv(Func) && !isCallingConvC)
+    if (!ignoreCallingConv(Func) && !IsCallingConvC)
       return nullptr;
     if (Value *V = optimizeStringMemoryLibCall(CI, Builder))
       return V;
     if (Value *V = optimizeFloatingPointLibCall(CI, Func, Builder))
       return V;
     switch (Func) {
     case LibFunc_ffs:
     case LibFunc_ffsl:
Value *FortifiedLibCallSimplifier::optimizeCall(CallInst *CI,
   //
   // Until we change clang and/or teach external users to check for availability
   // differently, disregard the "nobuiltin" attribute and TLI::has.
   //
   // PR23093.
 
   LibFunc Func;
   Function *Callee = CI->getCalledFunction();
-  bool isCallingConvC = isCallingConvCCompatible(CI);
+  bool IsCallingConvC = TargetLibraryInfoImpl::isCallingConvCCompatible(CI);
 
   SmallVector<OperandBundleDef, 2> OpBundles;
   CI->getOperandBundlesAsDefs(OpBundles);
 
   IRBuilderBase::OperandBundlesGuard Guard(Builder);
   Builder.setDefaultOperandBundles(OpBundles);
 
   // First, check that this is a known library functions and that the prototype
   // is correct.
   if (!TLI->getLibFunc(*Callee, Func))
     return nullptr;
 
   // We never change the calling convention.
-  if (!ignoreCallingConv(Func) && !isCallingConvC)
+  if (!ignoreCallingConv(Func) && !IsCallingConvC)
     return nullptr;
 
   switch (Func) {
   case LibFunc_memcpy_chk:
     return optimizeMemCpyChk(CI, Builder);
   case LibFunc_mempcpy_chk:
     return optimizeMemPCpyChk(CI, Builder);
   case LibFunc_memmove_chk:

llvm/test/Transforms/InstCombine/call-callconv-mismatch.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S | FileCheck %s
+; Verify that a cdecl-compatible calling convention does not trigger emitting
+; unreachable idom `store i1 true, i1* undef`.
+
+define arm_aapcs_vfpcc i8 @bar(i8* %0) {
+; CHECK-LABEL: @bar(
+; CHECK-NEXT:    [[TMP2:%.*]] = load i8, i8* [[TMP0:%.*]], align 1
+; CHECK-NEXT:    ret i8 [[TMP2]]
+;
+  %2 = load i8, i8* %0, align 1
+  ret i8 %2
+}
+
+define dso_local arm_aapcs_vfpcc i8 @foo(i8* %0) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:    [[TMP2:%.*]] = call i8 @bar(i8* [[TMP0:%.*]])
+; CHECK-NEXT:    ret i8 [[TMP2]]
+;
+  %2 = call i8 @bar(i8* %0)
+  ret i8 %2
+}