[llvm] r326638 - [ThinLTO] Revert r325320: Import global variables
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 2 16:10:22 PST 2018
On Fri, Mar 2, 2018 at 4:07 PM, Chandler Carruth <chandlerc at gmail.com>
wrote:
> 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.
>
Not an issue without this patch, as there was previously only one import
message. We can just recommit this along with the fixed global importing
patch.
Teresa
>
>
>>
>> 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/IR
>>> Mover.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/Transform
>>> s/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::GlobalVarK
>>> ind;
>>> - }
>>> - 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/Transfor
>>> ms/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/Transfor
>>> ms/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>
>>
>
--
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/20180302/7799203e/attachment.html>
More information about the llvm-commits
mailing list