Inlining of empty/small variadic functions.

[Sunil_Srivastava]

Summary

This is a fix for PR27796, inlining of empty/small variadic

While the general problem of inlining variadic functions is difficult, it is quite easy to inline variadic functions if they do not use va_start, because in those cases we can simply ignore arguments passed against '...'. That use pattern, though seemingly wasteful, is actually quite useful for writing functions for trace messages, with bodies that are removed via conditional compilation in production builds. This is done with the expectation that the compiler will inline these essentially empty functions and optimize out entire tracing overhead.

That is the purpose of this proposed change.

The determination of whether a the called function if using va_start is done at the time of inlining by scanning the code of the called function. However, in order to avoid the cost of this extra scan, we limit this scan to 'small' function; the definition of 'small' being that it must have a single block of no more that 20 instructions. Larger callees are rejected without full scan. That is sufficient for our use cases of mostly empty functions. The choice of 20 for number of instructions is somewhat arbitrary.

[chandlerc]

I think we should do this during the inline cost scan, which has a natural thresholding effect. That will also allow us to inline variadic functions where there is a use of 'va_start', but only behind a condition that is known to be false at the callsite we're inlining through.

There are a few other places where we bail out of the cost analysis if we see a construct that simply can't be handled, and it feels like this would fit well there.

[Sunil_Srivastava]

This is an update to inlining-of-variadic-functions change along the lines of suggestion by Chandler.

The determination to attempt or not is being made in InlineCost.cpp.

Presence of a va_start or a MustTail call in a variadic callee disqualifies it from being inlined. Other than that they can be inlined.

Changes in InlineFunction.cpp are merely for:

• to remove prohibition on inlining of variadic functions
• assertion checks that a variadic function having a va_start or MustTail do not slip through the cost analysis.

I do have one concern: Should I worry about plain 'tail' calls as well, same as 'musttail' calls ? Looking for opinions.

[chandlerc]

In D22529#556295, @Sunil_Srivastava wrote:

This is an update to inlining-of-variadic-functions change along the lines of suggestion by Chandler.

The determination to attempt or not is being made in InlineCost.cpp.

Presence of a va_start or a MustTail call in a variadic callee disqualifies it from being inlined. Other than that they can be inlined.

What's the motivation to inline cases where a variadic function calls some other variadic function but not with 'musttail'? I understand that such a call can't usefully call 'va_start', but such a call seems unlikely to matter regardless? If this is important to handle for some reason, I think it would be important to understand why. If it isn't important, it would be much simpler to just match the old behavior but inside inline cost where we can ignore uses along dead code paths.

I am a bit worried about C code with direct, trivial forwarding from one function to another without anything *explicitly* marking the inner call such that it is musttail, and just relying on a tail call occurring....

Some minor nits below, not sure they will be relevant depending on the answer to the issues above.

lib/Transforms/Utils/InlineFunction.cpp

1788

nit: wrong naming convention for variable...

1795–1798

nit: formatting is weird here.

[Sunil_Srivastava]

What's the motivation to inline cases where a variadic function calls some other variadic function but not with 'musttail'?

Hmm. I don't think I am looking for variadic function calling other variadic function; with or without 'musttail'.
Note that CallAnalyzer::FIsVarArg indicates whether CallAnalyzer::F is variadic, not whether any callee of CallAnalyzer::F is variadic. So if the call being considered for inlining is of A calling B, then the change being proposed only considers whether B is variadic, and if it is, then it also considers whether any call inside B is musttail.

Could you give an example of what you are concerned about here ?

I understand that such a call can't usefully call 'va_start', but such a call seems unlikely to matter regardless? If this is important to handle for some reason, I think it would be important to understand why. If it isn't important, it would be much simpler to just match the old behavior but inside inline cost where we can ignore uses along dead code paths.

We can address this after I understand exactly what your concern is.

I am a bit worried about C code with direct, trivial forwarding from one function to another without anything *explicitly* marking the inner call such that it is musttail, and just relying on a tail call occurring....

That one I am also concerned about, though I don't know why that would be a worry in C only. Any suggestions about it? Would setting such cloned calls to NoTail work ?

In the worst case we could just disqualify variadic callees having any calls irrespective of any tail-criteria. At least our use cases of interest are situation where the variadic callee is empty.

[Sunil_Srivastava]

ping

[Sunil_Srivastava]

A friendly ping please

[Sunil_Srivastava]

ping

[efriedma]

If it's taking more than a couple of weeks to get a response from a reviewer, you should email them directly; some people filter mail to llvm-commits into a separate mailbox, and don't notice pings on the mailing list.

[hfinkel]

lib/Analysis/InlineCost.cpp

914

Really? Does the LangRef guarantee this?

[Sunil_Srivastava]

lib/Analysis/InlineCost.cpp

914

First, I must admit that I do not fully know where musttail calls are used. I believe they are used for thunks.

LangRef does not even have the concept of musttail, so I can not answer your question precisely.

I am trying to be conservative and try to prevent C from accessing A-to-B unnamed arguments via some builtins or some other way in a situation like this:

void C(int x, ...) // actual user code.
{

va_start(ap, x); 
assert(va_arg(ap, int) == 5);

}
void B(int a, ...) // a thunk meant to jump to C
{

.. do something. Maybe a+=4 ..
C(a, ....); // musttail call somehow. Essentially a jump to C

}
void A(int x){

B(a, 5);

}

Now if we inline B into A, we will throw away 5 before C is even entered. Therefore I want to prevent B-into-A inlining.

[Sunil_Srivastava]

Took care of the 'nits' and updated to the current version of llvm.

No logical changes. Still waiting for response on logical points.

lib/Analysis/InlineCost.cpp

class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
   bool IsCallerRecursive;
   bool IsRecursiveCall;
   bool ExposesReturnsTwice;
   bool HasDynamicAlloca;
   bool ContainsNoDuplicateCall;
   bool HasReturn;
   bool HasIndirectBr;
   bool HasFrameEscape;
+  bool HasVaStart;      // whether F has va_start in it.
+  bool HasMustTailCall; // whether F has a tail or musttail call in it.
+  bool FIsVarArg;       // whether F is variadic by declaration.
 
   /// Number of bytes allocated statically by the callee.
   uint64_t AllocatedSize;
   unsigned NumInstructions, NumVectorInstructions;
   int FiftyPercentVectorBonus, TenPercentVectorBonus;
   int VectorBonus;
 
   /// While we walk the potentially-inlined instructions, we build up and
CallAnalyzer(const TargetTransformInfo &TTI,
                ProfileSummaryInfo *PSI, Function &Callee, CallSite CSArg,
                const InlineParams &Params)
       : TTI(TTI), GetAssumptionCache(GetAssumptionCache), GetBFI(GetBFI),
         PSI(PSI), F(Callee), DL(F.getParent()->getDataLayout()),
         CandidateCS(CSArg), Params(Params), Threshold(Params.DefaultThreshold),
         Cost(0), IsCallerRecursive(false), IsRecursiveCall(false),
         ExposesReturnsTwice(false), HasDynamicAlloca(false),
         ContainsNoDuplicateCall(false), HasReturn(false), HasIndirectBr(false),
-        HasFrameEscape(false), AllocatedSize(0), NumInstructions(0),
+        HasFrameEscape(false), HasVaStart(false), HasMustTailCall(false),
+        FIsVarArg(Callee.isVarArg()), AllocatedSize(0), NumInstructions(0),
         NumVectorInstructions(0), FiftyPercentVectorBonus(0),
         TenPercentVectorBonus(0), VectorBonus(0), NumConstantArgs(0),
         NumConstantOffsetPtrArgs(0), NumAllocaArgs(0), NumConstantPtrCmps(0),
         NumConstantPtrDiffs(0), NumInstructionsSimplified(0),
         SROACostSavings(0), SROACostSavingsLost(0) {}
 
   bool analyzeCall(CallSite CS);
 
bool CallAnalyzer::visitCallSite(CallSite CS) {
   if (CS.hasFnAttr(Attribute::ReturnsTwice) &&
       !F.hasFnAttribute(Attribute::ReturnsTwice)) {
     // This aborts the entire analysis.
     ExposesReturnsTwice = true;
     return false;
   }
   if (CS.isCall() && cast<CallInst>(CS.getInstruction())->cannotDuplicate())
     ContainsNoDuplicateCall = true;
+  if (CS.isCall()) {
+    auto CI = cast<CallInst>(CS.getInstruction());
+    if (CI->isMustTailCall()) {
+      HasMustTailCall = true;
+      // If A calls a variadic function B and B has a musttail call to C, then
+      // C can do va_start and access args passed from A to B against '...'.
+      // Therefore we can not inline B inside A.
+      if (FIsVarArg)
+        return false; // No point looking further.
+    }
+  }
 
   if (Function *F = CS.getCalledFunction()) {
     // When we have a concrete function, first try to simplify it directly.
     if (simplifyCallSite(F, CS))
       return true;
 
     // Next check if it is an intrinsic we know about.
     // FIXME: Lift this into part of the InstVisitor.
if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction())) {
       case Intrinsic::memset:
       case Intrinsic::memcpy:
       case Intrinsic::memmove:
         // SROA can usually chew through these intrinsics, but they aren't free.
         return false;
       case Intrinsic::localescape:
         HasFrameEscape = true;
         return false;
+      case Intrinsic::vastart:
+        // The callee has a va_start. We can not handle that in inlinining.
+        HasVaStart = true;
+        return false;
       }
     }
 
     if (F == CS.getInstruction()->getParent()->getParent()) {
       // This flag will fully abort the analysis, so don't bother with anything
       // else.
       IsRecursiveCall = true;
       return false;
for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I) {
     // cost should count against inlining.
     if (Base::visit(&*I))
       ++NumInstructionsSimplified;
     else
       Cost += InlineConstants::InstrCost;
 
     // If the visit this instruction detected an uninlinable pattern, abort.
     if (IsRecursiveCall || ExposesReturnsTwice || HasDynamicAlloca ||
-        HasIndirectBr || HasFrameEscape)
+        HasIndirectBr || HasFrameEscape || HasVaStart ||
+        (HasMustTailCall && FIsVarArg))
       return false;
 
     // If the caller is a recursive function then we don't want to inline
     // functions which allocate a lot of stack space because it would increase
     // the caller stack usage dramatically.
     if (IsCallerRecursive &&
         AllocatedSize > InlineConstants::TotalAllocaSizeRecursiveCaller)
       return false;
InlineCost llvm::getInlineCost(
   if (ShouldInline && CA.getCost() >= CA.getThreshold())
     return InlineCost::getAlways();
 
   return llvm::InlineCost::get(CA.getCost(), CA.getThreshold());
 }
 
 bool llvm::isInlineViable(Function &F) {
   bool ReturnsTwice = F.hasFnAttribute(Attribute::ReturnsTwice);
+  bool IsVarArg = F.isVarArg();
   for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {
     // Disallow inlining of functions which contain indirect branches or
     // blockaddresses.
     if (isa<IndirectBrInst>(BI->getTerminator()) || BI->hasAddressTaken())
       return false;
 
     for (auto &II : *BI) {
       CallSite CS(&II);
       if (!CS)
         continue;
 
+      if (IsVarArg) {
+        if (const CallInst *CI = dyn_cast<CallInst>(&II)) {
+          // Same logic as in analyzeBlock above.
+          if (CI->isMustTailCall())
+            return false;
+        }
+      }
+
       // Disallow recursive calls.
       if (&F == CS.getCalledFunction())
         return false;
 
       // Disallow calls which expose returns-twice to a function not previously
       // attributed as such.
       if (!ReturnsTwice && CS.isCall() &&
           cast<CallInst>(CS.getInstruction())->canReturnTwice())
         return false;
 
-      // Disallow inlining functions that call @llvm.localescape. Doing this
-      // correctly would require major changes to the inliner.
-      if (CS.getCalledFunction() &&
-          CS.getCalledFunction()->getIntrinsicID() ==
-              llvm::Intrinsic::localescape)
+      // Disallow inlining functions that call @llvm.localescape or @vastart.
+      // Doing this correctly would require major changes to the inliner.
+      if (auto CF = CS.getCalledFunction()) {
+        auto IntrId = CF->getIntrinsicID();
+        if (IntrId == Intrinsic::localescape || IntrId == Intrinsic::vastart)
           return false;
       }
     }
+  }
 
   return true;
 }
 
 // APIs to create InlineParams based on command line flags and/or other
 // parameters.
 
 InlineParams llvm::getInlineParams(int Threshold) {

lib/Transforms/Utils/InlineFunction.cpp

bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
   Instruction *TheCall = CS.getInstruction();
   assert(TheCall->getParent() && TheCall->getFunction()
          && "Instruction not in function!");
 
   // If IFI has any state in it, zap it before we fill it in.
   IFI.reset();
 
   Function *CalledFunc = CS.getCalledFunction();
-  if (!CalledFunc ||              // Can't inline external function or indirect
-      CalledFunc->isDeclaration() || // call, or call to a vararg function!
-      CalledFunc->getFunctionType()->isVarArg()) return false;
+  if (!CalledFunc || // Can't inline external function or indirect calls
+      CalledFunc->isDeclaration())
+    return false;
 
   // The inliner does not know how to inline through calls with operand bundles
   // in general ...
   if (CS.hasOperandBundles()) {
     for (int i = 0, e = CS.getNumOperandBundles(); i != e; ++i) {
       uint32_t Tag = CS.getOperandBundleAt(i).getTagID();
       // ... but it knows how to inline through "deopt" operand bundles ...
       if (Tag == LLVMContext::OB_deopt)
bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
 
   { // Scope to destroy VMap after cloning.
     ValueToValueMapTy VMap;
     // Keep a list of pair (dst, src) to emit byval initializations.
     SmallVector<std::pair<Value*, Value*>, 4> ByValInit;
 
     auto &DL = Caller->getParent()->getDataLayout();
 
+    if (CalledFunc->getFunctionType()->isVarArg())
+      assert(CalledFunc->arg_size() <= CS.arg_size() &&
+             "Not enough arguments passed to vararg function");
+    else
       assert(CalledFunc->arg_size() == CS.arg_size() &&
              "No varargs calls can be inlined!");
 
     // Calculate the vector of arguments to pass into the function cloner, which
     // matches up the formal to the actual argument values.
     CallSite::arg_iterator AI = CS.arg_begin();
     unsigned ArgNo = 0;
     for (Function::arg_iterator I = CalledFunc->arg_begin(),
          E = CalledFunc->arg_end(); I != E; ++I, ++AI, ++ArgNo) {
       Value *ActualArg = *AI;
bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
   }
 
   bool InlinedMustTailCalls = false, InlinedDeoptimizeCalls = false;
   if (InlinedFunctionInfo.ContainsCalls) {
     CallInst::TailCallKind CallSiteTailKind = CallInst::TCK_None;
     if (CallInst *CI = dyn_cast<CallInst>(TheCall))
       CallSiteTailKind = CI->getTailCallKind();
 
+    bool CalleeIsVararg = CalledFunc->isVarArg(); // Just for asserts below.
     for (Function::iterator BB = FirstNewBlock, E = Caller->end(); BB != E;
          ++BB) {
       for (Instruction &I : *BB) {
         CallInst *CI = dyn_cast<CallInst>(&I);
         if (!CI)
           continue;
+        if (CalleeIsVararg) {
+          assert((!CI->isMustTailCall()) &&
+                 "Inlined musttailcall in vararg function");
+        }
 
-        if (Function *F = CI->getCalledFunction())
+        if (Function *F = CI->getCalledFunction()) {
           InlinedDeoptimizeCalls |=
               F->getIntrinsicID() == Intrinsic::experimental_deoptimize;
+          if (CalleeIsVararg)
+            assert((F->getIntrinsicID() != Intrinsic::vastart) &&
+                   "Inlined vastart");
+        }
 
         // We need to reduce the strength of any inlined tail calls.  For
         // musttail, we have to avoid introducing potential unbounded stack
         // growth.  For example, if functions 'f' and 'g' are mutually recursive
         // with musttail, we can inline 'g' into 'f' so long as we preserve
         // musttail on the cloned call to 'f'.  If either the inlined call site
         // or the cloned call site is *not* musttail, the program already has
         // one frame of stack growth, so it's safe to remove musttail.  Here is