[GlobalOpt] don't change CC of musttail calle(e|r) PR36546

[indutny]

Summary

When the function has musttail call - its cc is fixed to be equal to the
cc of the musttail callee. In such case (and in the case of the musttail
callee), GlobalOpt should not change the cc to fastcc as it will break
the invariant.

[JDevlieghere]

Won't this interfere with tail call optimization? From the language manual, I understood it only to be supported for fastcc. (+ the ghc and HiPE calling conventions, which are already not considered here) If we can change the CC of the whole chain, the invariant would still hold. Would that be a valid alternative?

PS: Please upload full context diffs next time :-)

lib/Transforms/IPO/GlobalOpt.cpp

2137 ↗

(On Diff #136228)

Wouldn't it be cheaper to check the CC first, do an early return and only check for musttail calls/callees later?

[indutny]

Address inline comments.

---

As far as I can see it, the documentation doesn't say anything about musttail in particular. It says that tail calls require fastcc to be optimized, but even this seems to be rather stale.

Changing CC for the whole chain is totally a valid alternative. Would it make sense to you if I'll add a FIXME comment there?

[JDevlieghere]

In D43859#1022602, @indutny wrote:

Address inline comments.

---

As far as I can see it, the documentation doesn't say anything about musttail in particular. It says that tail calls require fastcc to be optimized, but even this seems to be rather stale.

Changing CC for the whole chain is totally a valid alternative. Would it make sense to you if I'll add a FIXME comment there?

Alright, that works for me! With the FIXME added this LGTM.

[indutny]

Added FIXME, as requested.

---

I'd appreciate if you could land this for me, as I don't have the commit bit yet.

[JDevlieghere]

The test case seems to fail for me locally. Can you see if you can reproduce this?

******************** TEST 'LLVM :: Transforms/GlobalOpt/musttail_cc.ll' FAILED ********************
Script:
--
/Users/jonas/llvm/git-svn/build/bin/opt < /Users/jonas/llvm/git-svn/llvm/test/Transforms/GlobalOpt/musttail_cc.ll -globalopt -S | /Users/jonas/llvm/git-svn/build/bin/FileCheck /Users/jonas/llvm/git-svn/llvm/test/Transforms/GlobalOpt/musttail_cc.ll
--
Exit Code: 1

Command Output (stderr):
--
/Users/jonas/llvm/git-svn/llvm/test/Transforms/GlobalOpt/musttail_cc.ll:31:16: error: expected string not found in input
; CHECK-LABEL: define internal i32 @foo2(i32 %a)
               ^
<stdin>:18:35: note: scanning from here
define internal i32 @foo1(i32 %a) unnamed_addr {
                                  ^
<stdin>:19:11: note: possible intended match here
 %ca = musttail call i32 @foo2(i32 %a)
          ^

--

[indutny]

Fix test.

---

Sorry, looks like I forgot to add internal to the foo2 (or removed it by mistake when rebasing).

[JDevlieghere]

Thanks Fedor!

llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp

 }
 
 /// Return true if this is a calling convention that we'd like to change.  The
 /// idea here is that we don't want to mess with the convention if the user
 /// explicitly requested something with performance implications like coldcc,
 /// GHC, or anyregcc.
 static bool hasChangeableCC(Function *F) {
   CallingConv::ID CC = F->getCallingConv();
+
   // FIXME: Is it worth transforming x86_stdcallcc and x86_fastcallcc?
-  return CC == CallingConv::C || CC == CallingConv::X86_ThisCall;
+  if (CC != CallingConv::C && CC != CallingConv::X86_ThisCall)
+    return false;
+
+  // FIXME: Change CC for the whole chain of musttail calls when possible.
+  //
+  // Can't change CC of the function that either has musttail calls, or is a
+  // musttail callee itself
+  for (User *U : F->users()) {
+    if (isa<BlockAddress>(U))
+      continue;
+    CallInst* CI = dyn_cast<CallInst>(U);
+    if (!CI)
+      continue;
+
+    if (CI->isMustTailCall())
+      return false;
+  }
+
+  for (BasicBlock &BB : *F)
+    if (BB.getTerminatingMustTailCall())
+      return false;
+
+  return true;
 }
 
 /// Return true if the block containing the call site has a BlockFrequency of
 /// less than ColdCCRelFreq% of the entry block.
 static bool isColdCallSite(CallSite CS, BlockFrequencyInfo &CallerBFI) {
   const BranchProbability ColdProb(ColdCCRelFreq, 100);
   auto CallSiteBB = CS.getInstruction()->getParent();
   auto CallSiteFreq = CallerBFI.getBlockFreq(CallSiteBB);

llvm/trunk/test/Transforms/GlobalOpt/musttail_cc.ll

+; RUN: opt < %s -globalopt -S | FileCheck %s
+; PR36546
+
+; Check that musttail callee preserves its calling convention
+
+define i32 @test(i32 %a) {
+  ; CHECK: %ca = musttail call i32 @foo(i32 %a)
+  %ca = musttail call i32 @foo(i32 %a)
+  ret i32 %ca
+}
+
+; CHECK-LABEL: define internal i32 @foo(i32 %a)
+define internal i32 @foo(i32 %a) {
+  ret i32 %a
+}
+
+; Check that musttail caller preserves its calling convention
+
+define i32 @test2(i32 %a) {
+  %ca = call i32 @foo1(i32 %a)
+  ret i32 %ca
+}
+
+; CHECK-LABEL: define internal i32 @foo1(i32 %a)
+define internal i32 @foo1(i32 %a) {
+  ; CHECK: %ca = musttail call i32 @foo2(i32 %a)
+  %ca = musttail call i32 @foo2(i32 %a)
+  ret i32 %ca
+}
+
+; CHECK-LABEL: define internal i32 @foo2(i32 %a)
+define internal i32 @foo2(i32 %a) {
+  ret i32 %a
+}