[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.
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

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