[ThinLTO] Add correctness check for RO/WO variable import
This patch adds an assertion check for exported read/write-only
variables to be also in import list for module. If they aren't
we may face linker errors, because read/write-only variables are
internalized in their source modules. The patch also changes
export lists to store ValueInfo instead of GUID for performance
considerations.
Differential revision: https://reviews.llvm.org/D70128
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index b22cf47..d5419e1 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -308,7 +308,7 @@
auto MarkExported = [&](const ValueInfo &VI, const GlobalValueSummary *S) {
if (ExportLists)
- (*ExportLists)[S->modulePath()].insert(VI.getGUID());
+ (*ExportLists)[S->modulePath()].insert(VI);
};
for (auto &RefSummary : VI.getSummaryList())
@@ -497,7 +497,7 @@
// Make exports in the source module.
if (ExportLists) {
auto &ExportList = (*ExportLists)[ExportModulePath];
- ExportList.insert(VI.getGUID());
+ ExportList.insert(VI);
if (!PreviouslyImported) {
// This is the first time this function was exported from its source
// module, so mark all functions and globals it references as exported
@@ -505,14 +505,11 @@
// For efficiency, we unconditionally add all the referenced GUIDs
// to the ExportList for this module, and will prune out any not
// defined in the module later in a single pass.
- for (auto &Edge : ResolvedCalleeSummary->calls()) {
- auto CalleeGUID = Edge.first.getGUID();
- ExportList.insert(CalleeGUID);
- }
- for (auto &Ref : ResolvedCalleeSummary->refs()) {
- auto GUID = Ref.getGUID();
- ExportList.insert(GUID);
- }
+ for (auto &Edge : ResolvedCalleeSummary->calls())
+ ExportList.insert(Edge.first);
+
+ for (auto &Ref : ResolvedCalleeSummary->refs())
+ ExportList.insert(Ref);
}
}
}
@@ -630,6 +627,37 @@
}
#endif
+static bool
+checkVariableImport(const ModuleSummaryIndex &Index,
+ StringMap<FunctionImporter::ImportMapTy> &ImportLists,
+ StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
+
+ DenseSet<GlobalValue::GUID> FlattenedImports;
+
+ for (auto &ImportPerModule : ImportLists)
+ for (auto &ExportPerModule : ImportPerModule.second)
+ FlattenedImports.insert(ExportPerModule.second.begin(),
+ ExportPerModule.second.end());
+
+ // Checks that all GUIDs of read/writeonly vars we see in export lists
+ // are also in the import lists. Otherwise we my face linker undefs,
+ // because readonly and writeonly vars are internalized in their
+ // source modules.
+ auto IsReadOrWriteOnlyVar = [&](StringRef ModulePath, const ValueInfo &VI) {
+ auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
+ Index.findSummaryInModule(VI, ModulePath));
+ return GVS && (Index.isReadOnly(GVS) || Index.isWriteOnly(GVS));
+ };
+
+ for (auto &ExportPerModule : ExportLists)
+ for (auto &VI : ExportPerModule.second)
+ if (!FlattenedImports.count(VI.getGUID()) &&
+ IsReadOrWriteOnlyVar(ExportPerModule.first(), VI))
+ return false;
+
+ return true;
+}
+
/// Compute all the import and export for every module using the Index.
void llvm::ComputeCrossModuleImport(
const ModuleSummaryIndex &Index,
@@ -654,14 +682,13 @@
for (auto &ELI : ExportLists) {
const auto &DefinedGVSummaries =
ModuleToDefinedGVSummaries.lookup(ELI.first());
- for (auto EI = ELI.second.begin(); EI != ELI.second.end();) {
- if (!DefinedGVSummaries.count(*EI))
- EI = ELI.second.erase(EI);
- else
- ++EI;
- }
+ std::remove_if(ELI.second.begin(), ELI.second.end(),
+ [&](const ValueInfo &VI) {
+ return !DefinedGVSummaries.count(VI.getGUID());
+ });
}
+ assert(checkVariableImport(Index, ImportLists, ExportLists));
#ifndef NDEBUG
LLVM_DEBUG(dbgs() << "Import/Export lists for " << ImportLists.size()
<< " modules:\n");
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index c667927..a000037 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -710,7 +710,7 @@
void updateIndexWPDForExports(
ModuleSummaryIndex &Summary,
- function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
+ function_ref<bool(StringRef, ValueInfo)> isExported,
std::map<ValueInfo, std::vector<VTableSlotSummary>> &LocalWPDTargetsMap) {
for (auto &T : LocalWPDTargetsMap) {
auto &VI = T.first;
@@ -718,7 +718,7 @@
assert(VI.getSummaryList().size() == 1 &&
"Devirt of local target has more than one copy");
auto &S = VI.getSummaryList()[0];
- if (!isExported(S->modulePath(), VI.getGUID()))
+ if (!isExported(S->modulePath(), VI))
continue;
// It's been exported by a cross module import.
diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
index 401be68..d795a0a 100644
--- a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -250,12 +250,12 @@
// contains summaries from the source modules if they are being imported.
// We might have a non-null VI and get here even in that case if the name
// matches one in this module (e.g. weak or appending linkage).
- auto* GVS = dyn_cast_or_null<GlobalVarSummary>(
- ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
+ auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
+ ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
if (GVS &&
(ImportIndex.isReadOnly(GVS) || ImportIndex.isWriteOnly(GVS))) {
V->addAttribute("thinlto-internalize");
- // Objects referenced by writeonly GV initializer should not be
+ // Objects referenced by writeonly GV initializer should not be
// promoted, because there is no any kind of read access to them
// on behalf of this writeonly GV. To avoid promotion we convert
// GV initializer to 'zeroinitializer'. This effectively drops