[llvm] r326638 - [ThinLTO] Revert r325320: Import global variables
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 2 16:03:54 PST 2018
On Fri, Mar 2, 2018 at 3:40 PM, Chandler Carruth via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: chandlerc
> Date: Fri Mar 2 15:40:08 2018
> New Revision: 326638
>
> URL: http://llvm.org/viewvc/llvm-project?rev=326638&view=rev
> Log:
> [ThinLTO] Revert r325320: Import global variables
>
> This caused some links to fail with ThinLTO due to missing symbols as
> well as causing some binaries to have failures at runtime. We're working
> with the author to get a test case, but want to get the tree green
> again.
>
> Further, it appears to introduce a data race. While the test usage of
> threads was disabled in r325361 & r325362, that isn't an acceptable fix.
> I've reverted both of these as well. This code needs to be thread safe.
> Test cases for this are already on the original commit thread.
>
FTR, the description of it being a data race is incorrect - it was just
interleaved debug/trace output that is being checked (i.e. the list of
imported functions/variables emitted under debug option in the backends).
The backend parallelism needs to be turned down to 1 to get deterministic
messages from the otherwise parallel backends.
Teresa
>
> Removed:
> llvm/trunk/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll
> llvm/trunk/test/ThinLTO/X86/globals-import-const-fold.ll
> Modified:
> llvm/trunk/lib/Linker/IRMover.cpp
> llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> llvm/trunk/test/Transforms/FunctionImport/funcimport.ll
> llvm/trunk/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll
>
> Modified: llvm/trunk/lib/Linker/IRMover.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/
> IRMover.cpp?rev=326638&r1=326637&r2=326638&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Linker/IRMover.cpp (original)
> +++ llvm/trunk/lib/Linker/IRMover.cpp Fri Mar 2 15:40:08 2018
> @@ -1051,10 +1051,14 @@ void IRLinker::prepareCompileUnitsForImp
> ValueMap.MD()[CU->getRawEnumTypes()].reset(nullptr);
> ValueMap.MD()[CU->getRawMacros()].reset(nullptr);
> ValueMap.MD()[CU->getRawRetainedTypes()].reset(nullptr);
> - // 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.
> + // 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<GlobalVariable>(GV); }) &&
> + "Unexpected importing of a GlobalVariable definition");
> ValueMap.MD()[CU->getRawGlobalVariables()].reset(nullptr);
>
> // Imported entities only need to be mapped in if they have local
>
> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/IPO/FunctionImport.cpp?rev=326638&r1=326637&r2=326638&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Fri Mar 2 15:40:08
> 2018
> @@ -61,7 +61,6 @@ 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");
> @@ -239,33 +238,6 @@ updateValueInfoForIndirectCalls(const Mo
> return Index.getValueInfo(GUID);
> }
>
> -static void computeImportForReferencedGlobals(
> - const FunctionSummary &Summary, const GVSummaryMapTy
> &DefinedGVSummaries,
> - FunctionImporter::ImportMapTy &ImportList,
> - StringMap<FunctionImporter::ExportSetTy> *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()) &&
> - // For now we don't import global variables which have outgoing
> - // refs. Otherwise we have to promote referenced vars/functions.
> - RefSummary->refs().empty()) {
> - ImportList[RefSummary->modulePath()][VI.getGUID()] = 1;
> - if (ExportLists)
> - (*ExportLists)[RefSummary->modulePath()].insert(VI.getGUID());
> - }
> - }
> -}
> -
> /// 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.
> @@ -275,8 +247,6 @@ static void computeImportForFunction(
> SmallVectorImpl<EdgeInfo> &Worklist,
> FunctionImporter::ImportMapTy &ImportList,
> StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr) {
> - computeImportForReferencedGlobals(Summary, DefinedGVSummaries,
> ImportList,
> - ExportLists);
> for (auto &Edge : Summary.calls()) {
> ValueInfo VI = Edge.first;
> DEBUG(dbgs() << " edge -> " << VI.getGUID() << " Threshold:" <<
> Threshold
> @@ -419,34 +389,6 @@ 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<const GlobalValue::GUID, unsigned> &P) {
> - return P.first;
> -}
> -
> -template <class T>
> -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,
> @@ -484,17 +426,12 @@ void llvm::ComputeCrossModuleImport(
> for (auto &ModuleImports : ImportLists) {
> auto ModName = ModuleImports.first();
> auto &Exports = ExportLists[ModName];
> - unsigned NumGVS = numGlobalVarSummaries(Index, Exports);
> - DEBUG(dbgs() << "* Module " << ModName << " exports "
> - << Exports.size() - NumGVS << " functions and " <<
> NumGVS
> - << " vars. Imports from "
> - << ModuleImports.second.size() << " modules.\n");
> + DEBUG(dbgs() << "* Module " << ModName << " exports " <<
> Exports.size()
> + << " functions. Imports from " <<
> ModuleImports.second.size()
> + << " modules.\n");
> for (auto &Src : ModuleImports.second) {
> auto SrcModName = Src.first();
> - unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second);
> - DEBUG(dbgs() << " - " << Src.second.size() - NumGVSPerMod
> - << " functions imported from " << SrcModName << "\n");
> - DEBUG(dbgs() << " - " << NumGVSPerMod << " global vars imported
> from "
> + DEBUG(dbgs() << " - " << Src.second.size() << " functions imported
> from "
> << SrcModName << "\n");
> }
> }
> @@ -502,17 +439,13 @@ void llvm::ComputeCrossModuleImport(
> }
>
> #ifndef NDEBUG
> -static void dumpImportListForModule(const ModuleSummaryIndex &Index,
> - StringRef ModulePath,
> +static void dumpImportListForModule(StringRef ModulePath,
> FunctionImporter::ImportMapTy
> &ImportList) {
> DEBUG(dbgs() << "* Module " << ModulePath << " imports from "
> << ImportList.size() << " modules.\n");
> for (auto &Src : ImportList) {
> auto SrcModName = Src.first();
> - unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second);
> - DEBUG(dbgs() << " - " << Src.second.size() - NumGVSPerMod
> - << " functions imported from " << SrcModName << "\n");
> - DEBUG(dbgs() << " - " << NumGVSPerMod << " vars imported from "
> + DEBUG(dbgs() << " - " << Src.second.size() << " functions imported
> from "
> << SrcModName << "\n");
> }
> }
> @@ -532,7 +465,7 @@ void llvm::ComputeCrossModuleImportForMo
> ComputeImportForModule(FunctionSummaryMap, Index, ImportList);
>
> #ifndef NDEBUG
> - dumpImportListForModule(Index, ModulePath, ImportList);
> + dumpImportListForModule(ModulePath, ImportList);
> #endif
> }
>
> @@ -559,7 +492,7 @@ void llvm::ComputeCrossModuleImportForMo
> ImportList[Summary->modulePath()][GUID] = 1;
> }
> #ifndef NDEBUG
> - dumpImportListForModule(Index, ModulePath, ImportList);
> + dumpImportListForModule(ModulePath, ImportList);
> #endif
> }
>
> @@ -837,7 +770,7 @@ Expected<bool> FunctionImporter::importF
> Module &DestModule, const FunctionImporter::ImportMapTy &ImportList) {
> DEBUG(dbgs() << "Starting import for Module "
> << DestModule.getModuleIdentifier() << "\n");
> - unsigned ImportedCount = 0, ImportedGVCount = 0;
> + unsigned ImportedCount = 0;
>
> IRMover Mover(DestModule);
> // Do the actual import of functions now, one Module at a time
> @@ -897,7 +830,7 @@ Expected<bool> FunctionImporter::importF
> if (Import) {
> if (Error Err = GV.materialize())
> return std::move(Err);
> - ImportedGVCount += GlobalsToImport.insert(&GV);
> + GlobalsToImport.insert(&GV);
> }
> }
> for (GlobalAlias &GA : SrcModule->aliases()) {
> @@ -954,14 +887,9 @@ Expected<bool> FunctionImporter::importF
> NumImportedModules++;
> }
>
> - NumImportedFunctions += (ImportedCount - ImportedGVCount);
> - NumImportedGlobalVars += ImportedGVCount;
> + NumImportedFunctions += ImportedCount;
>
> - DEBUG(dbgs() << "Imported " << ImportedCount - ImportedGVCount
> - << " functions for Module " << DestModule.
> getModuleIdentifier()
> - << "\n");
> - DEBUG(dbgs() << "Imported " << ImportedGVCount
> - << " global variables for Module "
> + DEBUG(dbgs() << "Imported " << ImportedCount << " functions for Module "
> << DestModule.getModuleIdentifier() << "\n");
> return ImportedCount;
> }
>
> Removed: llvm/trunk/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> ThinLTO/X86/Inputs/globals-import-cf-baz.ll?rev=326637&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll (original)
> +++ llvm/trunk/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll (removed)
> @@ -1,4 +0,0 @@
> -target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> -target triple = "x86_64-pc-linux-gnu"
> -
> - at baz = local_unnamed_addr constant i32 10, align 4
>
> Removed: llvm/trunk/test/ThinLTO/X86/globals-import-const-fold.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> ThinLTO/X86/globals-import-const-fold.ll?rev=326637&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/ThinLTO/X86/globals-import-const-fold.ll (original)
> +++ llvm/trunk/test/ThinLTO/X86/globals-import-const-fold.ll (removed)
> @@ -1,23 +0,0 @@
> -; 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"
> -
> - at 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
> -}
>
> Modified: llvm/trunk/test/Transforms/FunctionImport/funcimport.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/FunctionImport/funcimport.ll?rev=326638&r1=
> 326637&r2=326638&view=diff
> ============================================================
> ==================
> --- llvm/trunk/test/Transforms/FunctionImport/funcimport.ll (original)
> +++ llvm/trunk/test/Transforms/FunctionImport/funcimport.ll Fri Mar 2
> 15:40:08 2018
> @@ -7,8 +7,7 @@
> ; 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
> -; 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.
> +; "-stats" requires +Asserts.
> ; REQUIRES: asserts
>
> ; Test import with smaller instruction limit
> @@ -81,7 +80,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.{{.*}} = available_externally hidden global void ()*
> null
> +; CHECK-DAG: @P.llvm.{{.*}} = external hidden global void ()*
> ; CHECK-DAG: %0 = load void ()*, void ()** @P.llvm.
> ; CHECK-DAG: store void ()* @staticfunc2.llvm.{{.*}}, void ()** @P.llvm.
>
> @@ -108,8 +107,6 @@ 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.
> @@ -145,9 +142,3 @@ 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]]
>
> Modified: llvm/trunk/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/PGOProfile/thinlto_samplepgo_icp3.ll?rev=326638&
> r1=326637&r2=326638&view=diff
> ============================================================
> ==================
> --- llvm/trunk/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll
> (original)
> +++ llvm/trunk/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll Fri
> Mar 2 15:40:08 2018
> @@ -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 -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
> +; 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
> ; Make sure we import the promted indirectly called target
> ; IMPORTS: Import _ZL3foov.llvm.0
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
--
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180302/2efddfed/attachment-0001.html>
More information about the llvm-commits
mailing list