[ThinLTO] Update handling of vararg functions to match inliner

[tejohnson]

Summary

Previously we marked all vararg functions as non-inlinable in the
function summary, which prevented their importing. However, the
corresponding inliner restriction was loosened in r321940/r342675
to only apply to functions calling va_start. Adjust the summary
flag computation to match.

[davidxl]

lgtm

[efriedma]

This doesn't completely match what the inliner is doing. We actually can inline functions that call va_start in some cases: specifically, we can inline in cases where we can prove the call to va_start is dead at a particular callsite. Probably doesn't come up often enough to matter, though.

[tejohnson]

In D54270#1298848, @efriedma wrote:

This doesn't completely match what the inliner is doing. We actually can inline functions that call va_start in some cases: specifically, we can inline in cases where we can prove the call to va_start is dead at a particular callsite. Probably doesn't come up often enough to matter, though.

Thanks for the comment. I'm inclined to remove the vararg restrictions on importing altogether then. I had measured it without any restrictions for a large benchmark and it didn't have a significant impact on the import counts and binary size in any case.

lib/Analysis/ModuleSummaryAnalysis.cpp

static void computeFunctionSummary(
   ICallPromotionAnalysis ICallAnalysis;
   SmallPtrSet<const User *, 8> Visited;
 
   // Add personality function, prefix data and prologue data to function's ref
   // list.
   findRefEdges(Index, &F, RefEdges, Visited);
 
   bool HasInlineAsmMaybeReferencingInternal = false;
+  bool InitsVarArgs = false;
   for (const BasicBlock &BB : F)
     for (const Instruction &I : BB) {
       if (isa<DbgInfoIntrinsic>(I))
         continue;
+      if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I)) {
+        if (II->getIntrinsicID() == Intrinsic::vastart)
+          InitsVarArgs = true;
+      }
       ++NumInsts;
       findRefEdges(Index, &I, RefEdges, Visited);
       auto CS = ImmutableCallSite(&I);
       if (!CS)
         continue;
 
       const auto *CI = dyn_cast<CallInst>(&I);
       // Since we don't know exactly which local values are referenced in inline
static void computeFunctionSummary(
   bool NotEligibleForImport =
       NonRenamableLocal || HasInlineAsmMaybeReferencingInternal;
   GlobalValueSummary::GVFlags Flags(F.getLinkage(), NotEligibleForImport,
                                     /* Live = */ false, F.isDSOLocal());
   FunctionSummary::FFlags FunFlags{
       F.hasFnAttribute(Attribute::ReadNone),
       F.hasFnAttribute(Attribute::ReadOnly),
       F.hasFnAttribute(Attribute::NoRecurse), F.returnDoesNotAlias(),
-      // Inliner doesn't handle variadic functions.
+      // Inliner doesn't handle variadic functions with va_start calls.
       // FIXME: refactor this to use the same code that inliner is using.
-      F.isVarArg() ||
+      InitsVarArgs ||
           // Don't try to import functions with noinline attribute.
           F.getAttributes().hasFnAttribute(Attribute::NoInline)};
   auto FuncSummary = llvm::make_unique<FunctionSummary>(
       Flags, NumInsts, FunFlags, RefEdges.takeVector(),
       CallGraphEdges.takeVector(), TypeTests.takeVector(),
       TypeTestAssumeVCalls.takeVector(), TypeCheckedLoadVCalls.takeVector(),
       TypeTestAssumeConstVCalls.takeVector(),
       TypeCheckedLoadConstVCalls.takeVector());

test/Bitcode/thinlto-function-summary.ll

 ; RUN: opt -name-anon-globals -module-summary < %s | llvm-bcanalyzer -dump | FileCheck %s -check-prefix=BC
 ; RUN: opt -passes=name-anon-globals -module-summary < %s | llvm-bcanalyzer -dump | FileCheck %s -check-prefix=BC
 ; Check for summary block/records.
 
 ; BC: <SOURCE_FILENAME
 ; "h"
 ; BC-NEXT: <GLOBALVAR {{.*}} op0=0 op1=1
 ; "foo"
 ; BC-NEXT: <FUNCTION op0=1 op1=3
 ; "bar"
 ; BC-NEXT: <FUNCTION op0=4 op1=3
 ; "anon.[32 chars].0"
 ; BC-NEXT: <FUNCTION op0=7 op1=39
 ; "variadic"
 ; BC-NEXT: <FUNCTION op0=46 op1=8
+; "llvm.va_start"
+; BC-NEXT: <FUNCTION op0=54 op1=13
 ; "f"
-; BC-NEXT: <ALIAS op0=54 op1=1
+; BC-NEXT: <ALIAS op0=67 op1=1
 ; BC: <GLOBALVAL_SUMMARY_BLOCK
 ; BC-NEXT: <VERSION
 ; BC-NEXT: <PERMODULE {{.*}} op0=1 op1=0
 ; BC-NEXT: <PERMODULE {{.*}} op0=2 op1=0
 ; BC-NEXT: <PERMODULE {{.*}} op0=3 op1=7
-; BC-NEXT: <PERMODULE {{.*}} op0=4 op1=0 op2=1 op3=16
-; BC-NEXT: <ALIAS {{.*}} op0=5 op1=0 op2=3
+; Summary for @variadic has flags (op3) = 16 since non-inlinable owing to
+; va_start call.
+; flag is set due to va_start call.
+; BC-NEXT: <PERMODULE {{.*}} op0=4 op1=0 op2=4 op3=16
+; BC-NEXT: <ALIAS {{.*}} op0=6 op1=0 op2=3
 ; BC-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 ; BC: <STRTAB_BLOCK
-; BC-NEXT: blob data = 'hfoobaranon.{{................................}}.0variadicf{{.*}}'
+; BC-NEXT: blob data = 'hfoobaranon.{{................................}}.0variadicllvm.va_startf{{.*}}'
 
 
 ; RUN: opt -name-anon-globals -module-summary < %s | llvm-dis | FileCheck %s
 ; Check that this round-trips correctly.
 
 ; ModuleID = '<stdin>'
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
entry:
   store void()* @0, void()** @h
         br label %return
 
 return:         ; preds = %entry
         ret void
 }
 
 define i32 @variadic(...) {
+    %ap = alloca i8*, align 8
+    %ap.0 = bitcast i8** %ap to i8*
+    call void @llvm.va_start(i8* %ap.0)
     ret i32 42
 }
+
+declare void @llvm.va_start(i8*) nounwind

test/Transforms/FunctionImport/Inputs/funcimport.ll

 @globalvar = global i32 1, align 4
 @staticvar = internal global i32 1, align 4
 @staticconstvar = internal unnamed_addr constant [2 x i32] [i32 10, i32 20], align 4
 @commonvar = common global i32 0, align 4
 @P = internal global void ()* null, align 8
 
 @weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void (...)*)
 @analias = alias void (...), bitcast (void ()* @globalfunc2 to void (...)*)
 @linkoncealias = alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 
 define void @globalfunc1() #0 {
 entry:
   call void @funcwithpersonality()
-  call void (...) @variadic()
+  call void (...) @variadic_va_start()
   ret void
 }
 
 define void @globalfunc2() #0 {
 entry:
   ret void
 }
 
entry:
   call void @globalfunc2()
   call void @globalfunc2()
   call void @globalfunc2()
   call void @globalfunc2()
   call void @globalfunc2()
   ret void
 }
 
-; Variadic function should not be imported because inliner doesn't handle it.
-define void @variadic(...) {
+; Variadic function without va_start can be imported because inliner
+; can handle it.
+define void @variadic_no_va_start(...) {
     ret void
 }
 
+; Variadic function with va_start should not be imported because inliner
+; doesn't handle it.
+define void @variadic_va_start(...) {
+    %ap = alloca i8*, align 8
+    %ap.0 = bitcast i8** %ap to i8*
+    call void @llvm.va_start(i8* %ap.0)
+    ret void
+}
+
+declare void @llvm.va_start(i8*) nounwind

test/Transforms/FunctionImport/funcimport.ll

entry:
   %call = call i32 (...) @referencestatics()
   %call1 = call i32 (...) @referenceglobals()
   %call2 = call i32 (...) @referencecommon()
   call void (...) @setfuncptr()
   call void (...) @callfuncptr()
   call void (...) @weakfunc()
   call void (...) @linkoncefunc2()
   call void (...) @referencelargelinkonce()
-  call void (...) @variadic()
+  call void (...) @variadic_no_va_start()
+  call void (...) @variadic_va_start()
   ret i32 0
 }
 
 ; Won't import weak alias
 ; CHECK-DAG: declare void @weakalias
 declare void @weakalias(...) #1
 
 ; External alias imported as available_externally copy of aliasee

 ; Won't import linkonce func
 ; CHECK-DAG: declare void @linkoncefunc2(...)
 declare void @linkoncefunc2(...) #1
 
 ; INSTLIMDEF-DAG: Import funcwithpersonality
 ; INSTLIMDEF-DAG: define available_externally hidden void @funcwithpersonality.llvm.{{.*}}() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !thinlto_src_module !0 {
 ; INSTLIM5-DAG: declare hidden void @funcwithpersonality.llvm.{{.*}}()
 
-; CHECK-DAG: declare void @variadic(...)
-declare void @variadic(...)
+; We can import variadic functions without a va_start, since the inliner
+; can handle them.
+; INSTLIMDEF-DAG: Import variadic_no_va_start
+; CHECK-DAG: define available_externally void @variadic_no_va_start(...) !thinlto_src_module !0 {
+declare void @variadic_no_va_start(...)
+
+; We shouldn't import variadic functions without a va_start, since the inliner
+; cannot handle them.
+; CHECK-DAG: declare void @variadic_va_start(...)
+declare void @variadic_va_start(...)
 
 ; INSTLIMDEF-DAG: Import globalfunc2
-; INSTLIMDEF-DAG: 13 function-import - Number of functions imported
+; INSTLIMDEF-DAG: 14 function-import - Number of functions imported
 ; INSTLIMDEF-DAG: 4 function-import - Number of global variables imported
 
 ; CHECK-DAG: !0 = !{!"{{.*}}/Inputs/funcimport.ll"}
 
 ; The actual GUID values will depend on path to test.
 ; GUID-DAG: GUID {{.*}} is weakalias
 ; GUID-DAG: GUID {{.*}} is referenceglobals
 ; GUID-DAG: GUID {{.*}} is weakfunc

 ; GUID-DAG: GUID {{.*}} is analias
 ; GUID-DAG: GUID {{.*}} is globalvar
 ; GUID-DAG: GUID {{.*}} is referencecommon
 ; GUID-DAG: GUID {{.*}} is linkoncealias
 ; GUID-DAG: GUID {{.*}} is callfuncptr
 ; GUID-DAG: GUID {{.*}} is linkoncefunc
 
 ; DUMP:       Module [[M1:.*]] imports from 1 module
-; DUMP-NEXT:  13 functions imported from [[M2:.*]]
+; DUMP-NEXT:  14 functions imported from [[M2:.*]]
 ; DUMP-NEXT:  4 vars imported from [[M2]]
-; DUMP:       Imported 13 functions for Module [[M1]]
+; DUMP:       Imported 14 functions for Module [[M1]]
 ; DUMP-NEXT:  Imported 4 global variables for Module [[M1]]