[llvm] r347886 - [ThinLTO] Import local variables from the same module as caller
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 29 09:02:43 PST 2018
Author: tejohnson
Date: Thu Nov 29 09:02:42 2018
New Revision: 347886
URL: http://llvm.org/viewvc/llvm-project?rev=347886&view=rev
Log:
[ThinLTO] Import local variables from the same module as caller
Summary:
We can sometimes end up with multiple copies of a local variable that
have the same GUID in the index. This happens when there are local
variables with the same name that are in different source files having the
same name/path at compile time (but compiled into different bitcode objects).
In this case make sure we import the copy in the caller's module.
This enables importing both of the variables having the same GUID
(but which will have different promoted names since the module paths,
and therefore the module hashes, will be distinct).
Importing the wrong copy is particularly problematic for read only
variables, since we must import them as a local copy whenever
referenced. Otherwise we get undefs at link time.
Note that the llvm-lto.cpp and ThinLTOCodeGenerator changes are needed
for testing the distributed index case via clang, which will be sent as
a separate clang-side patch shortly. We were previously not doing the
dead code/read only computation before computing imports when testing
distributed index generation (like it was for testing importing and
other ThinLTO mechanisms alone).
Reviewers: evgeny777
Subscribers: mehdi_amini, inglorion, eraman, steven_wu, dexonsmith, dang, llvm-commits
Differential Revision: https://reviews.llvm.org/D55047
Added:
llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var1.ll
llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var2.ll
llvm/trunk/test/ThinLTO/X86/local_name_conflict_var.ll
Modified:
llvm/trunk/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp
llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll
llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll
llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
llvm/trunk/tools/llvm-lto/llvm-lto.cpp
Modified: llvm/trunk/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h (original)
+++ llvm/trunk/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h Thu Nov 29 09:02:42 2018
@@ -273,8 +273,8 @@ public:
/**
* Compute and emit the imported files for module at \p ModulePath.
*/
- static void emitImports(StringRef ModulePath, StringRef OutputName,
- ModuleSummaryIndex &Index);
+ void emitImports(Module &Module, StringRef OutputName,
+ ModuleSummaryIndex &Index);
/**
* Perform cross-module importing for the module identified by
@@ -285,8 +285,8 @@ public:
/**
* Compute the list of summaries needed for importing into module.
*/
- static void gatherImportedSummariesForModule(
- StringRef ModulePath, ModuleSummaryIndex &Index,
+ void gatherImportedSummariesForModule(
+ Module &Module, ModuleSummaryIndex &Index,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
/**
Modified: llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp (original)
+++ llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp Thu Nov 29 09:02:42 2018
@@ -661,37 +661,52 @@ void ThinLTOCodeGenerator::crossModuleIm
* Compute the list of summaries needed for importing into module.
*/
void ThinLTOCodeGenerator::gatherImportedSummariesForModule(
- StringRef ModulePath, ModuleSummaryIndex &Index,
+ Module &TheModule, ModuleSummaryIndex &Index,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex) {
auto ModuleCount = Index.modulePaths().size();
+ auto ModuleIdentifier = TheModule.getModuleIdentifier();
// Collect for each module the list of function it defines (GUID -> Summary).
StringMap<GVSummaryMapTy> ModuleToDefinedGVSummaries(ModuleCount);
Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
+ // Convert the preserved symbols set from string to GUID
+ auto GUIDPreservedSymbols = computeGUIDPreservedSymbols(
+ PreservedSymbols, Triple(TheModule.getTargetTriple()));
+
+ // Compute "dead" symbols, we don't want to import/export these!
+ computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
+
// Generate import/export list
StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
ExportLists);
- llvm::gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
- ImportLists[ModulePath],
- ModuleToSummariesForIndex);
+ llvm::gatherImportedSummariesForModule(
+ ModuleIdentifier, ModuleToDefinedGVSummaries,
+ ImportLists[ModuleIdentifier], ModuleToSummariesForIndex);
}
/**
* Emit the list of files needed for importing into module.
*/
-void ThinLTOCodeGenerator::emitImports(StringRef ModulePath,
- StringRef OutputName,
+void ThinLTOCodeGenerator::emitImports(Module &TheModule, StringRef OutputName,
ModuleSummaryIndex &Index) {
auto ModuleCount = Index.modulePaths().size();
+ auto ModuleIdentifier = TheModule.getModuleIdentifier();
// Collect for each module the list of function it defines (GUID -> Summary).
StringMap<GVSummaryMapTy> ModuleToDefinedGVSummaries(ModuleCount);
Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
+ // Convert the preserved symbols set from string to GUID
+ auto GUIDPreservedSymbols = computeGUIDPreservedSymbols(
+ PreservedSymbols, Triple(TheModule.getTargetTriple()));
+
+ // Compute "dead" symbols, we don't want to import/export these!
+ computeDeadSymbolsInIndex(Index, GUIDPreservedSymbols);
+
// Generate import/export list
StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
@@ -699,13 +714,13 @@ void ThinLTOCodeGenerator::emitImports(S
ExportLists);
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
- llvm::gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
- ImportLists[ModulePath],
- ModuleToSummariesForIndex);
+ llvm::gatherImportedSummariesForModule(
+ ModuleIdentifier, ModuleToDefinedGVSummaries,
+ ImportLists[ModuleIdentifier], ModuleToSummariesForIndex);
std::error_code EC;
- if ((EC =
- EmitImportsFiles(ModulePath, OutputName, ModuleToSummariesForIndex)))
+ if ((EC = EmitImportsFiles(ModuleIdentifier, OutputName,
+ ModuleToSummariesForIndex)))
report_fatal_error(Twine("Failed to open ") + OutputName +
" to save imports lists\n");
}
Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
+++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Thu Nov 29 09:02:42 2018
@@ -293,9 +293,21 @@ static void computeImportForReferencedGl
LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
+ // If this is a local variable, make sure we import the copy
+ // in the caller's module. The only time a local variable can
+ // share an entry in the index is if there is a local with the same name
+ // in another module that had the same source file name (in a different
+ // directory), where each was compiled in their own directory so there
+ // was not distinguishing path.
+ auto LocalNotInModule = [&](const GlobalValueSummary *RefSummary) -> bool {
+ return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
+ RefSummary->modulePath() != Summary.modulePath();
+ };
+
for (auto &RefSummary : VI.getSummaryList())
if (isa<GlobalVarSummary>(RefSummary.get()) &&
- canImportGlobalVar(RefSummary.get())) {
+ canImportGlobalVar(RefSummary.get()) &&
+ !LocalNotInModule(RefSummary.get())) {
auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
// Only update stat if we haven't already imported this variable.
if (ILI.second)
Modified: llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll (original)
+++ llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict1.ll Thu Nov 29 09:02:42 2018
@@ -3,6 +3,8 @@ source_filename = "local_name_conflict.c
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
+ at baz = internal constant i32 10, align 4
+
; Function Attrs: noinline nounwind uwtable
define i32 @a() {
entry:
@@ -13,5 +15,6 @@ entry:
; Function Attrs: noinline nounwind uwtable
define internal i32 @foo() {
entry:
- ret i32 1
+ %0 = load i32, i32* @baz, align 4
+ ret i32 %0
}
Modified: llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll (original)
+++ llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict2.ll Thu Nov 29 09:02:42 2018
@@ -3,6 +3,8 @@ source_filename = "local_name_conflict.c
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
+ at baz = internal constant i32 10, align 4
+
; Function Attrs: noinline nounwind uwtable
define i32 @b() {
entry:
@@ -13,5 +15,6 @@ entry:
; Function Attrs: noinline nounwind uwtable
define internal i32 @foo() {
entry:
- ret i32 2
+ %0 = load i32, i32* @baz, align 4
+ ret i32 %0
}
Added: llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var1.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var1.ll?rev=347886&view=auto
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var1.ll (added)
+++ llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var1.ll Thu Nov 29 09:02:42 2018
@@ -0,0 +1,13 @@
+; ModuleID = 'local_name_conflict_var.o'
+source_filename = "local_name_conflict_var.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at baz = internal global i32 10, align 4
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @a() {
+entry:
+ %0 = load i32, i32* @baz, align 4
+ ret i32 %0
+}
Added: llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var2.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var2.ll?rev=347886&view=auto
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var2.ll (added)
+++ llvm/trunk/test/ThinLTO/X86/Inputs/local_name_conflict_var2.ll Thu Nov 29 09:02:42 2018
@@ -0,0 +1,13 @@
+; ModuleID = 'local_name_conflict_var.o'
+source_filename = "local_name_conflict_var.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at baz = internal global i32 10, align 4
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @b() {
+entry:
+ %0 = load i32, i32* @baz, align 4
+ ret i32 %0
+}
Modified: llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll (original)
+++ llvm/trunk/test/ThinLTO/X86/local_name_conflict.ll Thu Nov 29 09:02:42 2018
@@ -1,5 +1,5 @@
; Test handling when two files with the same source file name contain
-; static functions with the same name (which will have the same GUID
+; static functions/variables with the same name (which will have the same GUID
; in the combined index.
; Do setup work for all below tests: generate bitcode and combined index
@@ -8,19 +8,22 @@
; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict2.ll -o %t3.bc
; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
-; This module will import b() which should cause the copy of foo from
+; This module will import b() which should cause the copy of foo and baz from
; that module (%t3.bc) to be imported. Check that the imported reference's
; promoted name matches the imported copy.
; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
-; IMPORT: call i32 @foo.llvm.[[HASH:[0-9]+]]
+; IMPORT: @baz.llvm.[[HASH:[0-9]+]] = available_externally hidden constant i32 10, align 4
+; IMPORT: call i32 @foo.llvm.[[HASH]]
; IMPORT: define available_externally hidden i32 @foo.llvm.[[HASH]]()
; The copy in %t2.bc should not be exported/promoted/renamed
; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=NOEXPORTSTATIC
+; NOEXPORTSTATIC: @baz = internal constant i32 10, align 4
; NOEXPORTSTATIC: define internal i32 @foo()
; Make sure foo is promoted and renamed without complaint in %t3.bc.
; RUN: llvm-lto -thinlto-action=promote %t3.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTSTATIC
+; EXPORTSTATIC: @baz.llvm.{{.*}} = hidden constant i32 10, align 4
; EXPORTSTATIC: define hidden i32 @foo.llvm.
; ModuleID = 'local_name_conflict_main.o'
Added: llvm/trunk/test/ThinLTO/X86/local_name_conflict_var.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/ThinLTO/X86/local_name_conflict_var.ll?rev=347886&view=auto
==============================================================================
--- llvm/trunk/test/ThinLTO/X86/local_name_conflict_var.ll (added)
+++ llvm/trunk/test/ThinLTO/X86/local_name_conflict_var.ll Thu Nov 29 09:02:42 2018
@@ -0,0 +1,32 @@
+; Test handling when two files with the same source file name contain
+; static read only variables with the same name (which will have the same GUID
+; in the combined index).
+
+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary -module-hash %s -o %t.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict_var1.ll -o %t2.bc
+; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict_var2.ll -o %t3.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
+
+; This module will import a() and b() which should cause the read only copy
+; of baz from each of those modules to be imported. Check that the both are
+; imported as local copies.
+; RUN: llvm-lto -thinlto-action=import -exported-symbol=main %t.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
+; IMPORT: @baz.llvm.{{.*}} = internal global i32 10
+; IMPORT: @baz.llvm.{{.*}} = internal global i32 10
+
+; ModuleID = 'local_name_conflict_var_main.o'
+source_filename = "local_name_conflict_var_main.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @main() {
+entry:
+ %call1 = call i32 (...) @a()
+ %call2 = call i32 (...) @b()
+ ret i32 0
+}
+
+declare i32 @a(...)
+declare i32 @b(...)
Modified: llvm/trunk/tools/llvm-lto/llvm-lto.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-lto/llvm-lto.cpp?rev=347886&r1=347885&r2=347886&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-lto/llvm-lto.cpp (original)
+++ llvm/trunk/tools/llvm-lto/llvm-lto.cpp Thu Nov 29 09:02:42 2018
@@ -562,11 +562,14 @@ private:
auto Index = loadCombinedIndex();
for (auto &Filename : InputFilenames) {
+ LLVMContext Ctx;
+ auto TheModule = loadModule(Filename, Ctx);
+
// Build a map of module to the GUIDs and summary objects that should
// be written to its index.
std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
- ThinLTOCodeGenerator::gatherImportedSummariesForModule(
- Filename, *Index, ModuleToSummariesForIndex);
+ ThinGenerator.gatherImportedSummariesForModule(*TheModule, *Index,
+ ModuleToSummariesForIndex);
std::string OutputName = OutputFilename;
if (OutputName.empty()) {
@@ -594,12 +597,14 @@ private:
auto Index = loadCombinedIndex();
for (auto &Filename : InputFilenames) {
+ LLVMContext Ctx;
+ auto TheModule = loadModule(Filename, Ctx);
std::string OutputName = OutputFilename;
if (OutputName.empty()) {
OutputName = Filename + ".imports";
}
OutputName = getThinLTOOutputFile(OutputName, OldPrefix, NewPrefix);
- ThinLTOCodeGenerator::emitImports(Filename, OutputName, *Index);
+ ThinGenerator.emitImports(*TheModule, OutputName, *Index);
}
}
More information about the llvm-commits
mailing list