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

1700

nit: wrong naming convention for variable...

1706–1709

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

903 ↗

(On Diff #72927)

Really? Does the LangRef guarantee this?

[Sunil_Srivastava]

lib/Analysis/InlineCost.cpp

903 ↗

(On Diff #72927)

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/Transforms/Utils/InlineFunction.cpp

bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
   Instruction *TheCall = CS.getInstruction();
   assert(TheCall->getParent() && TheCall->getParent()->getParent() &&
          "Instruction not in function!");
 
   // If IFI has any state in it, zap it before we fill it in.
   IFI.reset();
   
   const 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 || CalledFunc->isDeclaration())
+    return false; // Can't inline external functions or indirect calls.
+
+  bool varargFuncAllowed = false;
+  if (CalledFunc->getFunctionType()->isVarArg()) {
+    // The CalledFunc is variadic by declaration, but we can still inline it if
+    // it does not have a va_start. This is a common use pattern where bodies of
+    // functions meant for trace messages can be conditionally suppressed with
+    // an ifdef, with the expectation that the call will get inlined and all
+    // overhead of trace messages will get optimized out in production builds.
+
+    // This code looks into the CalledFunc to determine whether it is using
+    // va_start. However, in order to limit the cost of this scan we are going
+    // to analyze only small functions. Larger functions, including those
+    // having multiple blocks will be rejected without completing the scan. 
+    // Our target use cases will all meet this criteria of 'small'.
+
+    if (CalledFunc->size() > 1)
+      return false; // Do not analyze multi block functions.
+    int ic = 0;
+    for (auto &I : *(CalledFunc->begin())) {
+      if (ic++ > 20)
+        return false; // Callee too large. Reject.
+      if (const CallInst *CI = dyn_cast<CallInst>(&I)) {
+        if (CI->isMustTailCall()) {
+          // If the CalledFunc has a musttail call, it will jump to that, and
+          // the target of that jump can have a va_start. So we reject these.
+          return false;
+        }
+        if (const Function *F = CI->getCalledFunction()) {
+          if (F->getIntrinsicID() == Intrinsic::vastart)
+            return false; // Callee has a va_start. Reject.
+        }
+      }
+    }
+    varargFuncAllowed = true; // Only for an assertion check below.
+  }
 
   // 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 (varargFuncAllowed) {
+      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::const_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();
 
     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 (Function *F = CI->getCalledFunction())
           InlinedDeoptimizeCalls |=
               F->getIntrinsicID() == Intrinsic::experimental_deoptimize;
 
         // 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

test/Transforms/Inline/inline-varargs.ll

+; RUN: opt -inline -S %s | FileCheck %s
+
+; Check that the side effects of unused arguments happen.
+; CHECK: %call{{.*}}callThis
+
+; Verify that calleeY gets inlined. Call should not exist.
+; CHECK-NOT: %call{{.*}}calleeY
+
+; Verify that calleeN does not get inlined. Call should exist.
+; CHECK: %call{{.*}}calleeN
+
+; ModuleID = 'inl.cpp'
+source_filename = "inl.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.__va_list_tag = type { i32, i32, i8*, i8* }
+
+$_Z7calleeYiiPKcz = comdat any
+
+$_Z7calleeNiiPKcz = comdat any
+
+@.str = private unnamed_addr constant [4 x i8] c"abc\00", align 1
+
+; Function Attrs: nounwind
+define i32 @_Z6calleri(i32 %p) #0 {
+entry:
+  %p.addr = alloca i32, align 4
+  %r = alloca i32, align 4
+  %q = alloca i32, align 4
+  store i32 %p, i32* %p.addr, align 4
+  %call = call i32 @_Z8callThisv()
+  %call1 = call i32 (i32, i32, i8*, ...) @_Z7calleeYiiPKcz(i32 12, i32 34, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i32* %p.addr, i32 %call, i32 2, i32 3)
+  store i32 %call1, i32* %r, align 4
+  %0 = load i32, i32* %p.addr, align 4
+  %inc = add nsw i32 %0, 1
+  store i32 %inc, i32* %p.addr, align 4
+  %call2 = call i32 (i32, i32, i8*, ...) @_Z7calleeNiiPKcz(i32 112, i32 134, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i32* %p.addr, i32 2, i32 3)
+  store i32 %call2, i32* %q, align 4
+  %1 = load i32, i32* %r, align 4
+  %2 = load i32, i32* %q, align 4
+  %add = add nsw i32 %1, %2
+  ret i32 %add
+}
+
+; Function Attrs: inlinehint nounwind
+define linkonce_odr i32 @_Z7calleeYiiPKcz(i32 %a, i32 %b, i8* %m, ...) #1 comdat {
+entry:
+  %a.addr = alloca i32, align 4
+  %b.addr = alloca i32, align 4
+  %m.addr = alloca i8*, align 8
+  store i32 %a, i32* %a.addr, align 4
+  store i32 %b, i32* %b.addr, align 4
+  store i8* %m, i8** %m.addr, align 8
+  %0 = load i32, i32* %a.addr, align 4
+  %1 = load i32, i32* %b.addr, align 4
+  %add = add nsw i32 %0, %1
+  ret i32 %add
+}
+
+declare i32 @_Z8callThisv() #2
+
+; Function Attrs: inlinehint nounwind
+define linkonce_odr i32 @_Z7calleeNiiPKcz(i32 %a, i32 %b, i8* %m, ...) #3 comdat {
+entry:
+  %a.addr = alloca i32, align 4
+  %b.addr = alloca i32, align 4
+  %m.addr = alloca i8*, align 8
+  %ap = alloca [1 x %struct.__va_list_tag], align 16
+  store i32 %a, i32* %a.addr, align 4
+  store i32 %b, i32* %b.addr, align 4
+  store i8* %m, i8** %m.addr, align 8
+  %arraydecay = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %ap, i32 0, i32 0
+  %arraydecay1 = bitcast %struct.__va_list_tag* %arraydecay to i8*
+  call void @llvm.va_start(i8* %arraydecay1)
+  %0 = load i32, i32* %a.addr, align 4
+  %1 = load i32, i32* %b.addr, align 4
+  %add = add nsw i32 %0, %1
+  %arraydecay2 = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %ap, i32 0, i32 0
+  %gp_offset_p = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %arraydecay2, i32 0, i32 0
+  %gp_offset = load i32, i32* %gp_offset_p, align 16
+  %fits_in_gp = icmp ule i32 %gp_offset, 40
+  br i1 %fits_in_gp, label %vaarg.in_reg, label %vaarg.in_mem
+
+vaarg.in_reg:                                     ; preds = %entry
+  %2 = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %arraydecay2, i32 0, i32 3
+  %reg_save_area = load i8*, i8** %2, align 16
+  %3 = getelementptr i8, i8* %reg_save_area, i32 %gp_offset
+  %4 = bitcast i8* %3 to i32*
+  %5 = add i32 %gp_offset, 8
+  store i32 %5, i32* %gp_offset_p, align 16
+  br label %vaarg.end
+
+vaarg.in_mem:                                     ; preds = %entry
+  %overflow_arg_area_p = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %arraydecay2, i32 0, i32 2
+  %overflow_arg_area = load i8*, i8** %overflow_arg_area_p, align 8
+  %6 = bitcast i8* %overflow_arg_area to i32*
+  %overflow_arg_area.next = getelementptr i8, i8* %overflow_arg_area, i32 8
+  store i8* %overflow_arg_area.next, i8** %overflow_arg_area_p, align 8
+  br label %vaarg.end
+
+vaarg.end:                                        ; preds = %vaarg.in_mem, %vaarg.in_reg
+  %vaarg.addr = phi i32* [ %4, %vaarg.in_reg ], [ %6, %vaarg.in_mem ]
+  %7 = load i32, i32* %vaarg.addr, align 4
+  %add3 = add nsw i32 %add, %7
+  ret i32 %add3
+}
+
+; Function Attrs: nounwind
+declare void @llvm.va_start(i8*) #4
+
+attributes #0 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { inlinehint nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #3 = { inlinehint nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #4 = { nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 3.9.0 (trunk 275529)"}
+; Generated by 'clang -cc1 -S -inl.cpp -emit-llvm' with following file.
+; #include <stdarg.h>
+; inline int calleeY(int a, int b, const char *m, ...) {return a+b;}
+; inline int calleeN(int a, int b, const char *m, ...) {
+;   va_list ap;
+;   va_start(ap, m);
+;   return a+b + va_arg(ap, int);
+; }
+; int callThis();
+; int caller(int p)
+; {
+;   int r = calleeY(12, 34, "abc", &p, callThis(), 2, 3);
+;   p++;
+;   int q = calleeN(112, 134, "abc", &p, 2, 3);
+;   return r + q;
+; }
+