Don't import variadic functions

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.

Reviewers: eraman, mehdi_amini, tejohnson

Subscribers: mehdi_amini, llvm-commits

Differential Revision: https://reviews.llvm.org/D23339

llvm-svn: 278432

diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 45d9bf7..f4205a0 100644 (file)
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -104,11 +104,23 @@ public:
     /// 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:
@@ -175,6 +187,8 @@ public:
     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()); }

diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 73a30c6..2c32925 100644 (file)
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -720,7 +720,7 @@ static GlobalValue::LinkageTypes getDecodedLinkage(unsigned Val) {
   }
 }
 
-// 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
@@ -728,8 +728,9 @@ static GlobalValueSummary::GVFlags getDecodedGVSummaryFlags(uint64_t RawFlags,
   // 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) {

diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index dce175b..eadef18 100644 (file)
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -991,7 +991,7 @@ 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.

diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 7446a7d..c9d008e 100644 (file)
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -130,6 +130,10 @@ static bool eligibleForImport(const ModuleSummaryIndex &Index,
     // 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.

diff --git a/llvm/test/Bitcode/thinlto-function-summary.ll b/llvm/test/Bitcode/thinlto-function-summary.ll
index f3fb6c4..0ab967d 100644 (file)
--- a/llvm/test/Bitcode/thinlto-function-summary.ll
+++ b/llvm/test/Bitcode/thinlto-function-summary.ll
@@ -9,13 +9,17 @@
 ; 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.
@@ -56,3 +60,7 @@ entry:
 return:         ; preds = %entry
         ret void
 }
+
+define i32 @variadic(...) {
+    ret i32 42
+}

diff --git a/llvm/test/Transforms/FunctionImport/Inputs/funcimport.ll b/llvm/test/Transforms/FunctionImport/Inputs/funcimport.ll
index fa96b8e..c4ef37a 100644 (file)
--- a/llvm/test/Transforms/FunctionImport/Inputs/funcimport.ll
+++ b/llvm/test/Transforms/FunctionImport/Inputs/funcimport.ll
@@ -11,6 +11,7 @@
 define void @globalfunc1() #0 {
 entry:
   call void @funcwithpersonality()
+  call void (...) @variadic()
   ret void
 }
 
@@ -146,4 +147,8 @@ entry:
   ret void
 }
 
+; Variadic function should not be imported because inliner doesn't handle it.
+define void @variadic(...) {
+    ret void
+}
 

diff --git a/llvm/test/Transforms/FunctionImport/funcimport.ll b/llvm/test/Transforms/FunctionImport/funcimport.ll
index 2b80a08..97c1848 100644 (file)
--- a/llvm/test/Transforms/FunctionImport/funcimport.ll
+++ b/llvm/test/Transforms/FunctionImport/funcimport.ll
@@ -32,6 +32,7 @@ entry:
   call void (...) @weakfunc()
   call void (...) @linkoncefunc2()
   call void (...) @referencelargelinkonce()
+  call void (...) @variadic()
   ret i32 0
 }
 
@@ -105,6 +106,9 @@ declare void @linkoncefunc2(...) #1
 ; 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"}