[PATCH] D120867: [IROutliner] Adding support to properly handle musttail, swifttailcc and tailcc

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Yeah, it looks like there's a clang attribute, but it isn't used anywhere in the LLVM test suite.

https://clang.llvm.org/docs/AttributeReference.html#musttail



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2438
+/// Determine how many different exit paths exists for a region in the \ref
+/// OutlinableGroup \param CurrentGroup.  This is done by chekcing how many
+/// branches target BasicBlocks outside of the region.
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2729
+          
+          if(CI->getCallingConv() == CallingConv::SwiftTail ||
+             CI->getCallingConv() == CallingConv::Tail)
----------------
Nit: You could just return this rather than explicitly returning true/false


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2738
+      CurrentGroup.HasTailCC =
+          CandidateVec[0].frontInstruction()->getFunction()->getCallingConv();
+
----------------
- Can you add a comment explaining why we need to check the caller's CC as well?
- Would it be faster to check this first, and then do the `any_of`?
- What if every candidate except for the first is in a function with `swifttailcc` or `tailcc`? What happens then? Can you add a testcase for that?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2744
+    CurrentGroup.ContainsMustTailCall =
+        any_of(CandidateVec[0], [](IRInstructionData &ID) {
+          CallInst *CI = dyn_cast<CallInst>(ID.Inst);
----------------
Could pull `CandidateVec[0]` out into a variable?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2749
+
+          return CI->isMustTailCall();
+        });
----------------
Nit


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2809
+    // not, this means that there will be instructions that need to be executed
+    // after the outlined function resolves to handle the different exit paths.
+
----------------
I think this needs a testcase?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120867/new/

https://reviews.llvm.org/D120867