From 89e33d97598d406992e854affdec3dc5c5737d45 Mon Sep 17 00:00:00 2001 From: Eugene Leviant Date: Mon, 12 Mar 2018 10:30:50 +0000 Subject: [PATCH] [ThinLTO] Recommit of import global variables This wasreverted in r326638 due to link problems and fixed afterwards llvm-svn: 327254 --- lib/Linker/IRMover.cpp | 12 +-- lib/Transforms/IPO/FunctionImport.cpp | 95 ++++++++++++++++--- .../X86/Inputs/globals-import-cf-baz.ll | 4 + test/ThinLTO/X86/Inputs/globals-import.ll | 9 ++ test/ThinLTO/X86/globals-import-const-fold.ll | 23 +++++ test/ThinLTO/X86/globals-import.ll | 35 +++++++ test/Transforms/FunctionImport/funcimport.ll | 13 ++- .../PGOProfile/thinlto_samplepgo_icp3.ll | 2 +- 8 files changed, 170 insertions(+), 23 deletions(-) create mode 100644 test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll create mode 100644 test/ThinLTO/X86/Inputs/globals-import.ll create mode 100644 test/ThinLTO/X86/globals-import-const-fold.ll create mode 100644 test/ThinLTO/X86/globals-import.ll diff --git a/lib/Linker/IRMover.cpp b/lib/Linker/IRMover.cpp index ec15bbfc658..42081442db7 100644 --- a/lib/Linker/IRMover.cpp +++ b/lib/Linker/IRMover.cpp @@ -1051,14 +1051,10 @@ void IRLinker::prepareCompileUnitsForImport() { ValueMap.MD()[CU->getRawEnumTypes()].reset(nullptr); ValueMap.MD()[CU->getRawMacros()].reset(nullptr); ValueMap.MD()[CU->getRawRetainedTypes()].reset(nullptr); - // If we ever start importing global variable defs, we'll need to - // add their DIGlobalVariable to the globals list on the imported - // DICompileUnit. Confirm none are imported, and then we can - // map the list of global variables to nullptr. - assert(none_of( - ValuesToLink, - [](const GlobalValue *GV) { return isa(GV); }) && - "Unexpected importing of a GlobalVariable definition"); + // We import global variables only temporarily in order for instcombine + // and globalopt to perform constant folding and static constructor + // evaluation. After that elim-avail-extern will covert imported globals + // back to declarations, so we don't need debug info for them. ValueMap.MD()[CU->getRawGlobalVariables()].reset(nullptr); // Imported entities only need to be mapped in if they have local diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp index b68058cbeea..f759474e40f 100644 --- a/lib/Transforms/IPO/FunctionImport.cpp +++ b/lib/Transforms/IPO/FunctionImport.cpp @@ -61,6 +61,7 @@ using namespace llvm; #define DEBUG_TYPE "function-import" STATISTIC(NumImportedFunctions, "Number of functions imported"); +STATISTIC(NumImportedGlobalVars, "Number of global variables imported"); STATISTIC(NumImportedModules, "Number of modules imported from"); STATISTIC(NumDeadSymbols, "Number of dead stripped symbols in index"); STATISTIC(NumLiveSymbols, "Number of live symbols in index"); @@ -238,6 +239,32 @@ updateValueInfoForIndirectCalls(const ModuleSummaryIndex &Index, ValueInfo VI) { return Index.getValueInfo(GUID); } +static void computeImportForReferencedGlobals( + const FunctionSummary &Summary, const GVSummaryMapTy &DefinedGVSummaries, + FunctionImporter::ImportMapTy &ImportList, + StringMap *ExportLists) { + for (auto &VI : Summary.refs()) { + if (DefinedGVSummaries.count(VI.getGUID())) { + DEBUG(dbgs() << "Ref ignored! Target already in destination module.\n"); + continue; + } + + DEBUG(dbgs() << " ref -> " << VI.getGUID() << "\n"); + + for (auto &RefSummary : VI.getSummaryList()) + if (RefSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind && + // Don't try to import regular LTO summaries added to dummy module. + !RefSummary->modulePath().empty() && + !GlobalValue::isInterposableLinkage(RefSummary->linkage()) && + RefSummary->refs().empty()) { + ImportList[RefSummary->modulePath()][VI.getGUID()] = 1; + if (ExportLists) + (*ExportLists)[RefSummary->modulePath()].insert(VI.getGUID()); + break; + } + } +} + /// Compute the list of functions to import for a given caller. Mark these /// imported functions and the symbols they reference in their source module as /// exported from their source module. @@ -247,6 +274,8 @@ static void computeImportForFunction( SmallVectorImpl &Worklist, FunctionImporter::ImportMapTy &ImportList, StringMap *ExportLists = nullptr) { + computeImportForReferencedGlobals(Summary, DefinedGVSummaries, ImportList, + ExportLists); for (auto &Edge : Summary.calls()) { ValueInfo VI = Edge.first; DEBUG(dbgs() << " edge -> " << VI.getGUID() << " Threshold:" << Threshold @@ -389,6 +418,34 @@ static void ComputeImportForModule( } } +#ifndef NDEBUG +static bool isGlobalVarSummary(const ModuleSummaryIndex &Index, + GlobalValue::GUID G) { + if (const auto &VI = Index.getValueInfo(G)) { + auto SL = VI.getSummaryList(); + if (!SL.empty()) + return SL[0]->getSummaryKind() == GlobalValueSummary::GlobalVarKind; + } + return false; +} + +static GlobalValue::GUID getGUID(GlobalValue::GUID G) { return G; } + +static GlobalValue::GUID +getGUID(const std::pair &P) { + return P.first; +} + +template +unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index, T &Cont) { + unsigned NumGVS = 0; + for (auto &V : Cont) + if (isGlobalVarSummary(Index, getGUID(V))) + ++NumGVS; + return NumGVS; +} +#endif + /// Compute all the import and export for every module using the Index. void llvm::ComputeCrossModuleImport( const ModuleSummaryIndex &Index, @@ -426,12 +483,17 @@ void llvm::ComputeCrossModuleImport( for (auto &ModuleImports : ImportLists) { auto ModName = ModuleImports.first(); auto &Exports = ExportLists[ModName]; - DEBUG(dbgs() << "* Module " << ModName << " exports " << Exports.size() - << " functions. Imports from " << ModuleImports.second.size() - << " modules.\n"); + unsigned NumGVS = numGlobalVarSummaries(Index, Exports); + DEBUG(dbgs() << "* Module " << ModName << " exports " + << Exports.size() - NumGVS << " functions and " << NumGVS + << " vars. Imports from " + << ModuleImports.second.size() << " modules.\n"); for (auto &Src : ModuleImports.second) { auto SrcModName = Src.first(); - DEBUG(dbgs() << " - " << Src.second.size() << " functions imported from " + unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second); + DEBUG(dbgs() << " - " << Src.second.size() - NumGVSPerMod + << " functions imported from " << SrcModName << "\n"); + DEBUG(dbgs() << " - " << NumGVSPerMod << " global vars imported from " << SrcModName << "\n"); } } @@ -439,13 +501,17 @@ void llvm::ComputeCrossModuleImport( } #ifndef NDEBUG -static void dumpImportListForModule(StringRef ModulePath, +static void dumpImportListForModule(const ModuleSummaryIndex &Index, + StringRef ModulePath, FunctionImporter::ImportMapTy &ImportList) { DEBUG(dbgs() << "* Module " << ModulePath << " imports from " << ImportList.size() << " modules.\n"); for (auto &Src : ImportList) { auto SrcModName = Src.first(); - DEBUG(dbgs() << " - " << Src.second.size() << " functions imported from " + unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second); + DEBUG(dbgs() << " - " << Src.second.size() - NumGVSPerMod + << " functions imported from " << SrcModName << "\n"); + DEBUG(dbgs() << " - " << NumGVSPerMod << " vars imported from " << SrcModName << "\n"); } } @@ -465,7 +531,7 @@ void llvm::ComputeCrossModuleImportForModule( ComputeImportForModule(FunctionSummaryMap, Index, ImportList); #ifndef NDEBUG - dumpImportListForModule(ModulePath, ImportList); + dumpImportListForModule(Index, ModulePath, ImportList); #endif } @@ -492,7 +558,7 @@ void llvm::ComputeCrossModuleImportForModuleFromIndex( ImportList[Summary->modulePath()][GUID] = 1; } #ifndef NDEBUG - dumpImportListForModule(ModulePath, ImportList); + dumpImportListForModule(Index, ModulePath, ImportList); #endif } @@ -770,7 +836,7 @@ Expected FunctionImporter::importFunctions( Module &DestModule, const FunctionImporter::ImportMapTy &ImportList) { DEBUG(dbgs() << "Starting import for Module " << DestModule.getModuleIdentifier() << "\n"); - unsigned ImportedCount = 0; + unsigned ImportedCount = 0, ImportedGVCount = 0; IRMover Mover(DestModule); // Do the actual import of functions now, one Module at a time @@ -830,7 +896,7 @@ Expected FunctionImporter::importFunctions( if (Import) { if (Error Err = GV.materialize()) return std::move(Err); - GlobalsToImport.insert(&GV); + ImportedGVCount += GlobalsToImport.insert(&GV); } } for (GlobalAlias &GA : SrcModule->aliases()) { @@ -887,9 +953,14 @@ Expected FunctionImporter::importFunctions( NumImportedModules++; } - NumImportedFunctions += ImportedCount; + NumImportedFunctions += (ImportedCount - ImportedGVCount); + NumImportedGlobalVars += ImportedGVCount; - DEBUG(dbgs() << "Imported " << ImportedCount << " functions for Module " + DEBUG(dbgs() << "Imported " << ImportedCount - ImportedGVCount + << " functions for Module " << DestModule.getModuleIdentifier() + << "\n"); + DEBUG(dbgs() << "Imported " << ImportedGVCount + << " global variables for Module " << DestModule.getModuleIdentifier() << "\n"); return ImportedCount; } diff --git a/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll b/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll new file mode 100644 index 00000000000..e41163063b4 --- /dev/null +++ b/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll @@ -0,0 +1,4 @@ +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-linux-gnu" + +@baz = local_unnamed_addr constant i32 10, align 4 diff --git a/test/ThinLTO/X86/Inputs/globals-import.ll b/test/ThinLTO/X86/Inputs/globals-import.ll new file mode 100644 index 00000000000..b229f4a4bde --- /dev/null +++ b/test/ThinLTO/X86/Inputs/globals-import.ll @@ -0,0 +1,9 @@ +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-linux-gnu" + +@baz = internal constant i32 10, align 4 + +define linkonce_odr i32 @foo() { + %1 = load i32, i32* @baz, align 4 + ret i32 %1 +} diff --git a/test/ThinLTO/X86/globals-import-const-fold.ll b/test/ThinLTO/X86/globals-import-const-fold.ll new file mode 100644 index 00000000000..49e31b79a47 --- /dev/null +++ b/test/ThinLTO/X86/globals-import-const-fold.ll @@ -0,0 +1,23 @@ +; RUN: opt -module-summary %s -o %t1.bc +; RUN: opt -module-summary %p/Inputs/globals-import-cf-baz.ll -o %t2.bc +; RUN: llvm-lto -thinlto-action=thinlink %t1.bc %t2.bc -o %t3.index.bc + +; RUN: llvm-lto -thinlto-action=import %t1.bc %t2.bc -thinlto-index=%t3.index.bc +; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck --check-prefix=IMPORT %s +; RUN: llvm-lto -thinlto-action=optimize %t1.bc.thinlto.imported.bc -o %t1.bc.thinlto.opt.bc +; RUN: llvm-dis %t1.bc.thinlto.opt.bc -o - | FileCheck --check-prefix=OPTIMIZE %s + +; IMPORT: @baz = available_externally local_unnamed_addr constant i32 10 + +; OPTIMIZE: define i32 @main() +; OPTIMIZE-NEXT: ret i32 10 + +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-linux-gnu" + +@baz = external local_unnamed_addr constant i32, align 4 + +define i32 @main() local_unnamed_addr { + %1 = load i32, i32* @baz, align 4 + ret i32 %1 +} diff --git a/test/ThinLTO/X86/globals-import.ll b/test/ThinLTO/X86/globals-import.ll new file mode 100644 index 00000000000..9fe1ebefe8b --- /dev/null +++ b/test/ThinLTO/X86/globals-import.ll @@ -0,0 +1,35 @@ +; Test to ensure that we import a single copy of a global variable. This is +; important when we link in an object file twice, which is normally works when +; all symbols have either weak or internal linkage. If we import an internal +; global variable twice it will get promoted in each module, and given the same +; name as the IR hash will be identical, resulting in multiple defs when linking. +; RUN: opt -module-summary %s -o %t1.bc +; RUN: opt -module-summary %p/Inputs/globals-import.ll -o %t2.bc +; RUN: opt -module-summary %p/Inputs/globals-import.ll -o %t2b.bc +; RUN: llvm-lto -thinlto-action=thinlink %t1.bc %t2.bc %t2b.bc -o %t3.index.bc + +; RUN: llvm-lto -thinlto-action=import %t1.bc -thinlto-index=%t3.index.bc +; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck --check-prefix=IMPORT %s +; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t3.index.bc +; RUN: llvm-lto -thinlto-action=promote %t2b.bc -thinlto-index=%t3.index.bc +; RUN: llvm-dis %t2.bc.thinlto.promoted.bc -o - | FileCheck --check-prefix=PROMOTE1 %s +; RUN: llvm-dis %t2b.bc.thinlto.promoted.bc -o - | FileCheck --check-prefix=PROMOTE2 %s + +; IMPORT: @baz.llvm.0 = available_externally hidden constant i32 10, align 4 + +; PROMOTE1: @baz.llvm.0 = hidden constant i32 10, align 4 +; PROMOTE1: define weak_odr i32 @foo() { + +; Second copy of IR object should not have any symbols imported/promoted. +; PROMOTE2: @baz = internal constant i32 10, align 4 +; PROMOTE2: define available_externally i32 @foo() { + +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-linux-gnu" + +declare i32 @foo() + +define i32 @main() local_unnamed_addr { + %1 = call i32 @foo() + ret i32 %1 +} diff --git a/test/Transforms/FunctionImport/funcimport.ll b/test/Transforms/FunctionImport/funcimport.ll index 4ff51a33b5e..bb974b587e3 100644 --- a/test/Transforms/FunctionImport/funcimport.ll +++ b/test/Transforms/FunctionImport/funcimport.ll @@ -7,7 +7,8 @@ ; RUN: opt -function-import -stats -print-imports -enable-import-metadata -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIMDEF ; Try again with new pass manager ; RUN: opt -passes='function-import' -stats -print-imports -enable-import-metadata -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIMDEF -; "-stats" requires +Asserts. +; RUN: opt -passes='function-import' -debug-only=function-import -enable-import-metadata -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=DUMP +; "-stats" and "-debug-only" require +Asserts. ; REQUIRES: asserts ; Test import with smaller instruction limit @@ -80,7 +81,7 @@ declare void @callfuncptr(...) #1 ; Ensure that all uses of local variable @P which has used in setfuncptr ; and callfuncptr are to the same promoted/renamed global. -; CHECK-DAG: @P.llvm.{{.*}} = external hidden global void ()* +; CHECK-DAG: @P.llvm.{{.*}} = available_externally hidden global void ()* null ; CHECK-DAG: %0 = load void ()*, void ()** @P.llvm. ; CHECK-DAG: store void ()* @staticfunc2.llvm.{{.*}}, void ()** @P.llvm. @@ -107,6 +108,8 @@ declare void @variadic(...) ; INSTLIMDEF-DAG: Import globalfunc2 ; INSTLIMDEF-DAG: 13 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. @@ -142,3 +145,9 @@ declare void @variadic(...) ; 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: 4 vars imported from [[M2]] +; DUMP: Imported 13 functions for Module [[M1]] +; DUMP-NEXT: Imported 4 global variables for Module [[M1]] diff --git a/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll b/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll index d6a94ca6d94..3044964f33c 100644 --- a/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll +++ b/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll @@ -6,7 +6,7 @@ ; Test to make sure importing and dead stripping works in the ; case where the target is a local function that also indirectly calls itself. -; RUN: llvm-lto2 run -save-temps -o %t3 %t.bc %t2.bc -r %t.bc,fptr,plx -r %t.bc,main,plx -r %t2.bc,_Z6updatei,pl -r %t2.bc,fptr,l -print-imports 2>&1 | FileCheck %s --check-prefix=IMPORTS +; RUN: llvm-lto2 run -thinlto-threads=1 -save-temps -o %t3 %t.bc %t2.bc -r %t.bc,fptr,plx -r %t.bc,main,plx -r %t2.bc,_Z6updatei,pl -r %t2.bc,fptr,l -print-imports 2>&1 | FileCheck %s --check-prefix=IMPORTS ; Make sure we import the promted indirectly called target ; IMPORTS: Import _ZL3foov.llvm.0