From 2ede9108727dc351ceebfc077815f6454a00c1e0 Mon Sep 17 00:00:00 2001 From: Piotr Padlewski Date: Thu, 11 Aug 2016 22:13:57 +0000 Subject: [PATCH] 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 --- include/llvm/IR/ModuleSummaryIndex.h | 20 ++++++++++++++++--- lib/Bitcode/Reader/BitcodeReader.cpp | 7 ++++--- lib/Bitcode/Writer/BitcodeWriter.cpp | 2 +- lib/Transforms/IPO/FunctionImport.cpp | 4 ++++ test/Bitcode/thinlto-function-summary.ll | 14 ++++++++++--- .../FunctionImport/Inputs/funcimport.ll | 5 +++++ test/Transforms/FunctionImport/funcimport.ll | 4 ++++ 7 files changed, 46 insertions(+), 10 deletions(-) diff --git a/include/llvm/IR/ModuleSummaryIndex.h b/include/llvm/IR/ModuleSummaryIndex.h index 45d9bf7af70..f4205a0d835 100644 --- a/include/llvm/IR/ModuleSummaryIndex.h +++ b/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(&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/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp index 73a30c61eca..2c3292527f5 100644 --- a/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/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/lib/Bitcode/Writer/BitcodeWriter.cpp b/lib/Bitcode/Writer/BitcodeWriter.cpp index dce175bc1d1..eadef182446 100644 --- a/lib/Bitcode/Writer/BitcodeWriter.cpp +++ b/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/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp index 7446a7d0f47..c9d008ebd38 100644 --- a/lib/Transforms/IPO/FunctionImport.cpp +++ b/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/test/Bitcode/thinlto-function-summary.ll b/test/Bitcode/thinlto-function-summary.ll index f3fb6c4f010..0ab967dfcad 100644 --- a/test/Bitcode/thinlto-function-summary.ll +++ b/test/Bitcode/thinlto-function-summary.ll @@ -9,13 +9,17 @@ ; BC-NEXT: record string = 'anon. +; BC-NEXT: record string = 'variadic' ; BC-NEXT: record string = 'foo' ; BC-NEXT: record string = 'bar' -; BC-NEXT: record string = 'f' +; BC-NEXT: record string = 'f' +; BC-NEXT: 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/test/Transforms/FunctionImport/Inputs/funcimport.ll b/test/Transforms/FunctionImport/Inputs/funcimport.ll index fa96b8ea266..c4ef37a168f 100644 --- a/test/Transforms/FunctionImport/Inputs/funcimport.ll +++ b/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/test/Transforms/FunctionImport/funcimport.ll b/test/Transforms/FunctionImport/funcimport.ll index 2b80a0827b5..97c18488af6 100644 --- a/test/Transforms/FunctionImport/funcimport.ll +++ b/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"}