[llvm] r326638 - [ThinLTO] Revert r325320: Import global variables
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 2 16:07:06 PST 2018
On Fri, Mar 2, 2018 at 4:03 PM Teresa Johnson <tejohnson at google.com> wrote:
> 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.
>
(thanks again for the clarification)
Does this need to be re-added? IE, will it be an issue even w/o the
underlying patch? Happy to do so if you recommend.
>
> 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 <(408)%20460-2413>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180303/7611f4ad/attachment.html>
More information about the llvm-commits
mailing list