Don't import variadic functions

[Prazek]

Summary

This patch adds IsVariadicFunction bit to summary in order
to not import variadic functions. Inliner doesn't inline
variadic functions because it is hard to reason about it.

This one small fix improves Importer by about 16%
(going from 86% to 100% of imported functions that are
inlined anywhere)
on some spec benchmarks like 'int' and others.

[Prazek]

format

[mehdi_amini]

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

[eraman]

In D23339#510677, @mehdi_amini wrote:

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

[Prazek]

In D23339#510690, @eraman wrote:

In D23339#510677, @mehdi_amini wrote:

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

[Prazek]

In D23339#510677, @mehdi_amini wrote:

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

Good point Mehdi

[Prazek]

In D23339#510712, @Prazek wrote:

In D23339#510690, @eraman wrote:

In D23339#510677, @mehdi_amini wrote:

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

I looked on this function, and it seems that inlineCost only calls it to determinate if function marked as "always_inline" should be inlined. I should add if Variadic there, but I don't think we should use this function to determinate the bit.

[eraman]

In D23339#510714, @Prazek wrote:

In D23339#510712, @Prazek wrote:

In D23339#510690, @eraman wrote:

In D23339#510677, @mehdi_amini wrote:

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

I looked on this function, and it seems that inlineCost only calls it to determinate if function marked as "always_inline" should be inlined. I should add if Variadic there, but I don't think we should use this function to determinate the bit.

In D23339#510714, @Prazek wrote:

In D23339#510712, @Prazek wrote:

In D23339#510690, @eraman wrote:

In D23339#510677, @mehdi_amini wrote:

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

I looked on this function, and it seems that inlineCost only calls it to determinate if function marked as "always_inline" should be inlined. I should add if Variadic there, but I don't think we should use this function to determinate the bit.

Yes, I missed the part in getInlineCost where this is called only with the always_inline attribute. All (or most) of the checks in this method is also checked in the regular inliner (SimpleInliner) but only in the reachable path. But it's probably fine not to check for them (and let the inliner reject them later). Even if you're checking for just var_args, this code be added to InlineCost.cpp.

[Prazek]

changed flag name

[mehdi_amini]

lib/Bitcode/Reader/BitcodeReader.cpp

733 ↗

(On Diff #67585)

Isn't it reversed here? The bit set to one is *not* viable to inline if I read correctly the writer counterpart.
There is something I'm probably missing, otherwise how's the test passing?

[Prazek]

lib/Bitcode/Reader/BitcodeReader.cpp

733 ↗

(On Diff #67585)

good catch, I just forgot to change this name. I firstly wanted to make flag "IsViableToInline", but then because this bit changed from 0 to 1, most tests were invalid.
I talked with Teresa and she said that it will be better to make this flag reverse, so it won't be set in most cases.

[mehdi_amini]

lib/Bitcode/Reader/BitcodeReader.cpp

733 ↗

(On Diff #67585)

So right now the test is *not* passing right? (Probably none of the ThinLTO test should pass with this)

[Prazek]

fixed variable name

[Prazek]

lib/Bitcode/Reader/BitcodeReader.cpp

733 ↗

(On Diff #67585)

no, tests totally pass. This variable name was the only bug here.

[mehdi_amini]

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

lib/Bitcode/Reader/BitcodeReader.cpp

733 ↗

(On Diff #67605)

Oh I see, the ! is in isViableToInline()

[tejohnson]

LGTM with a couple nits.

include/llvm/IR/ModuleSummaryIndex.h

189 ↗

(On Diff #67605)

Make this also isNotViableToInline, so that we don't negate here and then again at the callsite of this method.

lib/Bitcode/Reader/BitcodeReader.cpp

724 ↗

(On Diff #67605)

The Version flag is there in case we ever add flags such that older versions of bitcode need to be handled specially. E.g. if you had left this as a positive IsViableToInline, we would probably want to bump the version and set that flag when we had an older version of bitcode (so we didn't completely stop importing when compiling with older bitcode). Since the reversed flag indicates when not to import (and it is a heuristic and not correctness), I don't think there is any need to bump/check the version.

The upshot is that the FIXME can be removed.

[eraman]

In D23339#511869, @mehdi_amini wrote:

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

This check should be eventually moved to InlineCost so that importer and inliner are out of sync. For now, a FIXME should be useful.

[mehdi_amini]

In D23339#511890, @eraman wrote:

In D23339#511869, @mehdi_amini wrote:

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

This check should be eventually moved to InlineCost so that importer and inliner are out of sync. For now, a FIXME should be useful.

Very good point indeed.

[Prazek]

In D23339#511891, @mehdi_amini wrote:

In D23339#511890, @eraman wrote:

In D23339#511869, @mehdi_amini wrote:

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

This check should be eventually moved to InlineCost so that importer and inliner are out of sync. For now, a FIXME should be useful.

Very good point indeed.

So we could check all attributes but they are so not frequent that it wont make huge difference like this one. If the checks are in O(1) then it probably make sense to move them eventually, but things like traversing all instructions to check if function is recursive and other would not make much sense as these functions or instructions are not that frequent (imho)

[Prazek]

lib/Bitcode/Reader/BitcodeReader.cpp

724 ↗

(On Diff #67605)

I see. maybe I should change Version => /*Version, not used right now*/

[Prazek]

fixes

llvm/trunk/include/llvm/IR/ModuleSummaryIndex.h

struct GVFlags {
     /// index, to disambiguate from other values with the same name.
     /// In the future this will be used to update and optimize linkage
     /// types based on global summary-based analysis.
     unsigned Linkage : 4;
 
     /// Indicate if the global value is located in a specific section.
     unsigned HasSection : 1;
 
+    /// Indicate if the function is not viable to inline.
+    unsigned IsNotViableToInline : 1;
+
     /// Convenience Constructors
-    explicit GVFlags(GlobalValue::LinkageTypes Linkage, bool HasSection)
-        : Linkage(Linkage), HasSection(HasSection) {}
+    explicit GVFlags(GlobalValue::LinkageTypes Linkage, bool HasSection,
+                     bool IsNotViableToInline)
+        : Linkage(Linkage), HasSection(HasSection),
+          IsNotViableToInline(IsNotViableToInline) {}
+
     GVFlags(const GlobalValue &GV)
-        : Linkage(GV.getLinkage()), HasSection(GV.hasSection()) {}
+        : Linkage(GV.getLinkage()), HasSection(GV.hasSection()) {
+      IsNotViableToInline = false;
+      if (const auto *F = dyn_cast<Function>(&GV))
+        // Inliner doesn't handle variadic functions.
+        // FIXME: refactor this to use the same code that inliner is using.
+        IsNotViableToInline = F->isVarArg();
+    }
   };
 
 private:
   /// Kind of summary for use in dyn_cast<> et al.
   SummaryKind Kind;
 
   /// This is the hash of the name of the symbol in the original file. It is
   /// identical to the GUID for global symbols, but differs for local since the
public:
   }
 
   /// Sets the linkage to the value determined by global summary-based
   /// optimization. Will be applied in the ThinLTO backends.
   void setLinkage(GlobalValue::LinkageTypes Linkage) {
     Flags.Linkage = Linkage;
   }
 
+  bool isNotViableToInline() const { return Flags.IsNotViableToInline; }
+
   /// Return true if this summary is for a GlobalValue that needs promotion
   /// to be referenced from another module.
   bool needsRenaming() const { return GlobalValue::isLocalLinkage(linkage()); }
 
   /// Return true if this global value is located in a specific section.
   bool hasSection() const { return Flags.HasSection; }
 
   /// Record a reference from this global value to the global value identified

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

static GlobalValue::LinkageTypes getDecodedLinkage(unsigned Val) {
   case 18:
     return GlobalValue::LinkOnceAnyLinkage;
   case 11: // Old value with implicit comdat.
   case 19:
     return GlobalValue::LinkOnceODRLinkage;
   }
 }
 
-// Decode the flags for GlobalValue in the summary
+/// Decode the flags for GlobalValue in the summary.
 static GlobalValueSummary::GVFlags getDecodedGVSummaryFlags(uint64_t RawFlags,
                                                             uint64_t Version) {
   // Summary were not emitted before LLVM 3.9, we don't need to upgrade Linkage
   // like getDecodedLinkage() above. Any future change to the linkage enum and
   // to getDecodedLinkage() will need to be taken into account here as above.
   auto Linkage = GlobalValue::LinkageTypes(RawFlags & 0xF); // 4 bits
   RawFlags = RawFlags >> 4;
-  auto HasSection = RawFlags & 0x1; // bool
-  return GlobalValueSummary::GVFlags(Linkage, HasSection);
+  bool HasSection = RawFlags & 0x1;
+  bool IsNotViableToInline = RawFlags & 0x2;
+  return GlobalValueSummary::GVFlags(Linkage, HasSection, IsNotViableToInline);
 }
 
 static GlobalValue::VisibilityTypes getDecodedVisibility(unsigned Val) {
   switch (Val) {
   default: // Map unknown visibilities to default.
   case 0: return GlobalValue::DefaultVisibility;
   case 1: return GlobalValue::HiddenVisibility;
   case 2: return GlobalValue::ProtectedVisibility;

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

static unsigned getEncodedLinkage(const GlobalValue &GV) {
   return getEncodedLinkage(GV.getLinkage());
 }
 
 // Decode the flags for GlobalValue in the summary
 static uint64_t getEncodedGVSummaryFlags(GlobalValueSummary::GVFlags Flags) {
   uint64_t RawFlags = 0;
 
   RawFlags |= Flags.HasSection; // bool
-
+  RawFlags |= (Flags.IsNotViableToInline << 1);
   // Linkage don't need to be remapped at that time for the summary. Any future
   // change to the getEncodedLinkage() function will need to be taken into
   // account here as well.
   RawFlags = (RawFlags << 4) | Flags.Linkage; // 4 bits
 
   return RawFlags;
 }
 

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

 // module.
 static bool eligibleForImport(const ModuleSummaryIndex &Index,
                               const GlobalValueSummary &Summary) {
   if (!canBeExternallyReferenced(Summary))
     // Can't import a global that needs renaming if has a section for instance.
     // FIXME: we may be able to import it by copying it without promotion.
     return false;
 
+  // Don't import functions that are not viable to inline.
+  if (Summary.isNotViableToInline())
+    return false;
+
   // Check references (and potential calls) in the same module. If the current
   // value references a global that can't be externally referenced it is not
   // eligible for import.
   bool AllRefsCanBeExternallyReferenced =
       llvm::all_of(Summary.refs(), [&](const ValueInfo &VI) {
         return canBeExternallyReferenced(Index, VI.getGUID());
       });
   if (!AllRefsCanBeExternallyReferenced)

llvm/trunk/test/Bitcode/thinlto-function-summary.ll

 ; RUN: opt -name-anon-functions -module-summary < %s | llvm-bcanalyzer -dump | FileCheck %s -check-prefix=BC
 ; Check for summary block/records.
 
 ; Check the value ids in the summary entries against the
 ; same in the ValueSumbolTable, to ensure the ordering is stable.
 ; Also check the linkage field on the summary entries.
 ; 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: <ALIAS {{.*}} op0=4 op1=0 op2=3
+; BC-NEXT: <PERMODULE {{.*}} op0=4 op1=32
+; BC-NEXT: <ALIAS {{.*}} op0=5 op1=0 op2=3
 ; BC-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 ; BC-NEXT: <VALUE_SYMTAB
-; BC-NEXT: <FNENTRY {{.*}} op0=3 {{.*}}> record string = 'anon.
+; BC-NEXT: <FNENTRY {{.*}} op0=4 {{.*}}> record string = 'variadic'
 ; BC-NEXT: <FNENTRY {{.*}} op0=1 {{.*}}> record string = 'foo'
 ; BC-NEXT: <FNENTRY {{.*}} op0=2 {{.*}}> record string = 'bar'
-; BC-NEXT: <FNENTRY {{.*}} op0=4 {{.*}}> record string = 'f'
+; BC-NEXT: <FNENTRY {{.*}} op0=5 {{.*}}> record string = 'f'
+; BC-NEXT: <ENTRY {{.*}} record string = 'h'
+; BC-NEXT: <FNENTRY {{.*}} op0=3 {{.*}}> record string = 'anon.
+
 
 ; RUN: opt -name-anon-functions -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"
 

 define internal void @0() nounwind {
 entry:
   store void()* @0, void()** @h
         br label %return
 
 return:         ; preds = %entry
         ret void
 }
+
+define i32 @variadic(...) {
+    ret i32 42
+}

llvm/trunk/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()
   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(...) {
+    ret void
+}
 

llvm/trunk/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()
   ret i32 0
 }
 
 ; Won't import weak alias
 ; CHECK-DAG: declare void @weakalias
 declare void @weakalias(...) #1
 
 ; Cannot create an alias to available_externally

 ; 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(...)
+
 ; INSTLIMDEF-DAG: Import globalfunc2
 ; INSTLIMDEF-DAG: 13 function-import - Number of functions 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