[llvm] r325320 - [ThinLTO] Import global variables
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 2 15:43:22 PST 2018
Sorry for the trouble, but this patch introduced multiple failures for us
with ThinLTO. We're seeing link failures due to missing symbols as well as
runtime failures.
+Teresa Johnson <tejohnson at google.com> is going to work on getting test
cases for these, but it'll probably be Monday or Tuesday when we can make
progress there. For now, I've reverted this in r326638 so that the tree is
back to green.
Note that I'm also deeply troubled by the data race mentioned. Turning off
threads doesn't fix a data race, it just hides it. The code really needs to
be made thread safe and that should probably happen prior to re-committing
the patch. I've reverted r325361 and r325362 as well for now, but they
probably shouldn't be re-introduced and instead the data race issue fixed.
Thanks,
-Chandler
On Fri, Feb 16, 2018 at 8:58 AM Evgeny Leviant via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> See r325361
> ________________________________________
> От: David Green <David.Green at arm.com>
> Отправлено: 16 февраля 2018 г. 19:37
> Кому: Evgeny Leviant; llvm-commits
> Копия: nd
> Тема: Re: [llvm] r325320 - [ThinLTO] Import global variables
>
> Hello again,
>
> Looking through the llvm-lto2 help, perhaps adding -thinlto-threads=1 to
> this specific test is all we need? At least to get the output correct.
>
> That seems to work here, it just depends whether -print-imports should be
> thread safe or not?
> Dave
>
> ________________________________________
> From: David Green
> Sent: 16 February 2018 16:20:15
> To: Evgeny Leviant; llvm-commits
> Cc: nd
> Subject: Re: [llvm] r325320 - [ThinLTO] Import global variables
>
> Yeah, no probs. Like I said the error seems quite rare. If I spam this
> command then it eventually fails (but it may not on your machine if it's an
> thread-timing problem) (plus, sorry for the paths):
>
> /work/llvm-git/build/bin/llvm-lto2 run -save-temps -o
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp3
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp.bc
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp2.bc
> -r
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp.bc,fptr,plx
> -r
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp.bc,main,plx
> -r
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp2.bc,_Z6updatei,pl
> -r
> /work/llvm-git/build/test/Transforms/PGOProfile/Output/thinlto_samplepgo_icp3.ll.tmp2.bc,fptr,l
> -print-imports 2>&1 | tee output.txt | /work/llvm-git/build/bin/FileCheck
> /work/llvm-git/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll
> --check-prefix=IMPORTS
>
> I just noticed that before this commit, it would print:
> /work/llvm-git/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll:
> Import _ZL3foov.llvm.0 from
> /work/llvm-git/test/Transforms/PGOProfile/Inputs/thinlto_samplepgo_icp3.ll
>
> i.e no second output ("fptr") for it to interleave.
>
> I've attached the thinlto_samplepgo_icp3.ll.tmp.bc and
> thinlto_samplepgo_icp3.ll.tmp2.bc files, but I don't think they change
> between passing and failing runs, it's just the output of the llvm-lto2
> that is different.
>
> Let me know if I can do anything else,
> Thanks,
> Dave
>
>
> From: Evgeny Leviant <eleviant at accesssoftek.com>
> Sent: 16 February 2018 16:06
> To: David Green; llvm-commits
> Cc: nd
> Subject: Re: [llvm] r325320 - [ThinLTO] Import global variables
>
> Hello David,
>
> I got single buildbot failure today, but I can't reproduce this failure
> locally
> Can you please send test artifacts (BC files, e.t.c) when it fails? Thanks.
> ________________________________________
> От: David Green <David.Green at arm.com>
> Отправлено: 16 февраля 2018 г. 19:04
> Кому: Evgeny Leviant; llvm-commits
> Копия: nd
> Тема: Re: [llvm] r325320 - [ThinLTO] Import global variables
>
> Hello Eugene,
>
> After this patch, about 1 time in 20 I'm seeing failures in
> Transforms/PGOProfile/thinlto_samplepgo_icp3.ll. When it passes it prints
> something like:
> /work/llvm-git/test/Transforms/PGOProfile/Inputs/thinlto_samplepgo_icp3.ll:
> Import fptr from
> /work/llvm-git/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll
> /work/llvm-git/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll:
> Import _ZL3foov.llvm.0 from
> /work/llvm-git/test/Transforms/PGOProfile/Inputs/thinlto_samplepgo_icp3.ll
>
> When it fails it prints:
> /work/llvm-git/test/Transforms/PGOProfile/Inputs/thinlto_samplepgo_icp3.ll/work/llvm-git/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll:
> Import : Import fptr_ZL3foov.llvm.0 from
> /work/llvm-git/test/Transforms/PGOProfile/thinlto_samplepgo_icp3.ll from
> /work/llvm-git/test/Transforms/PGOProfile/Inputs/thinlto_samplepgo_icp3.ll
>
> I cant reproduce before this patch, but don't see anything very thread-y
> in here that may be causing this. Perhaps some knock-on affect? Perhaps
> it's printing more output now?
>
> Any ideas?
> Thanks,
> Dave
>
>
>
> From: llvm-commits <llvm-commits-bounces at lists.llvm.org> on behalf of
> Eugene Leviant via llvm-commits <llvm-commits at lists.llvm.org>
> Sent: 16 February 2018 08:11
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] r325320 - [ThinLTO] Import global variables
>
> Author: evgeny777
> Date: Fri Feb 16 00:11:04 2018
> New Revision: 325320
>
> URL: http://llvm.org/viewvc/llvm-project?rev=325320&view=rev
> Log:
> [ThinLTO] Import global variables
>
> Differential revision: https://reviews.llvm.org/D43077
>
> Added:
> 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
>
> Modified: llvm/trunk/lib/Linker/IRMover.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/IRMover.cpp?rev=325320&r1=325319&r2=325320&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Linker/IRMover.cpp (original)
> +++ llvm/trunk/lib/Linker/IRMover.cpp Fri Feb 16 00:11:04 2018
> @@ -1051,14 +1051,10 @@ void IRLinker::prepareCompileUnitsForImp
> 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<GlobalVariable>(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
>
> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=325320&r1=325319&r2=325320&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Fri Feb 16 00:11:04
> 2018
> @@ -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,33 @@ 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.
> @@ -247,6 +275,8 @@ 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
> @@ -389,6 +419,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<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,
> @@ -426,12 +484,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 +502,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 +532,7 @@ void llvm::ComputeCrossModuleImportForMo
> ComputeImportForModule(FunctionSummaryMap, Index, ImportList);
>
> #ifndef NDEBUG
> - dumpImportListForModule(ModulePath, ImportList);
> + dumpImportListForModule(Index, ModulePath, ImportList);
> #endif
> }
>
> @@ -492,7 +559,7 @@ void llvm::ComputeCrossModuleImportForMo
> ImportList[Summary->modulePath()][GUID] = 1;
> }
> #ifndef NDEBUG
> - dumpImportListForModule(ModulePath, ImportList);
> + dumpImportListForModule(Index, ModulePath, ImportList);
> #endif
> }
>
> @@ -770,7 +837,7 @@ Expected<bool> FunctionImporter::importF
> 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 +897,7 @@ Expected<bool> FunctionImporter::importF
> 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 +954,14 @@ Expected<bool> FunctionImporter::importF
> 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;
> }
>
> Added: 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=325320&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll (added)
> +++ llvm/trunk/test/ThinLTO/X86/Inputs/globals-import-cf-baz.ll Fri Feb 16
> 00:11:04 2018
> @@ -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"
> +
> + at baz = local_unnamed_addr constant i32 10, align 4
>
> Added: 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=325320&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/ThinLTO/X86/globals-import-const-fold.ll (added)
> +++ llvm/trunk/test/ThinLTO/X86/globals-import-const-fold.ll Fri Feb 16
> 00:11:04 2018
> @@ -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"
> +
> + 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=325320&r1=325319&r2=325320&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/FunctionImport/funcimport.ll (original)
> +++ llvm/trunk/test/Transforms/FunctionImport/funcimport.ll Fri Feb 16
> 00:11:04 2018
> @@ -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]]
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180302/60bfd258/attachment.html>
More information about the llvm-commits
mailing list