[llvm] a2292cc - [ThinLTO] Add correctness check for RO/WO variable import
Benjamin Kramer via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 14 07:09:01 PST 2019
I'm seeing a lot of linker errors during clang selfhost with ThinLTO
after this change. I reverted it in
360f661733245ec15be4fc10c413f683c3cdd13f, let me know if you need help
reproducing.
ld.lld: error: undefined hidden symbol: guard variable for
llvm::getTheHexagonTarget()::TheHexagonTarget
(.llvm.10283769002260223989)
>>> referenced by HexagonTargetInfo.cpp:14 (llvm/lib/Target/Hexagon/TargetInfo/HexagonTargetInfo.cpp:14)
>>> cc1_main.o:(llvm::InitializeAllTargets())
On Thu, Nov 14, 2019 at 10:28 AM via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> Author: evgeny
> Date: 2019-11-14T12:24:05+03:00
> New Revision: a2292cc537b561416c21e8d4017715d652c144cc
>
> URL: https://github.com/llvm/llvm-project/commit/a2292cc537b561416c21e8d4017715d652c144cc
> DIFF: https://github.com/llvm/llvm-project/commit/a2292cc537b561416c21e8d4017715d652c144cc.diff
>
> LOG: [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
>
> Added:
>
>
> Modified:
> llvm/include/llvm/LTO/LTO.h
> llvm/include/llvm/Transforms/IPO/FunctionImport.h
> llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
> llvm/lib/LTO/LTO.cpp
> llvm/lib/LTO/ThinLTOCodeGenerator.cpp
> llvm/lib/Transforms/IPO/FunctionImport.cpp
> llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
> llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/LTO/LTO.h b/llvm/include/llvm/LTO/LTO.h
> index 0a1e3e1d0e42..42c9d5b78c0b 100644
> --- a/llvm/include/llvm/LTO/LTO.h
> +++ b/llvm/include/llvm/LTO/LTO.h
> @@ -59,7 +59,7 @@ void thinLTOResolvePrevailingInIndex(
> /// must apply the changes to the Module via thinLTOInternalizeModule.
> void thinLTOInternalizeAndPromoteInIndex(
> ModuleSummaryIndex &Index,
> - function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
> + function_ref<bool(StringRef, ValueInfo)> isExported,
> function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
> isPrevailing);
>
>
> diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
> index bbf270c400af..b4dde7b199ff 100644
> --- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
> +++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
> @@ -98,7 +98,7 @@ class FunctionImporter {
> using ImportMapTy = StringMap<FunctionsToImportTy>;
>
> /// The set contains an entry for every global value the module exports.
> - using ExportSetTy = std::unordered_set<GlobalValue::GUID>;
> + using ExportSetTy = DenseSet<ValueInfo>;
>
> /// A function of this type is used to load modules referenced by the index.
> using ModuleLoaderTy =
>
> diff --git a/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h b/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
> index 22435e4ed1e5..8af2af7f352f 100644
> --- a/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
> +++ b/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
> @@ -251,7 +251,7 @@ void runWholeProgramDevirtOnIndex(
> /// devirt target names for any locals that were exported.
> void updateIndexWPDForExports(
> ModuleSummaryIndex &Summary,
> - function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
> + function_ref<bool(StringRef, ValueInfo)> isExported,
> std::map<ValueInfo, std::vector<VTableSlotSummary>> &LocalWPDTargetsMap);
>
> } // end namespace llvm
>
> diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
> index 1e345e7dd89e..9c59ec40274e 100644
> --- a/llvm/lib/LTO/LTO.cpp
> +++ b/llvm/lib/LTO/LTO.cpp
> @@ -146,9 +146,11 @@ void llvm::computeLTOCacheKey(
> // Include the hash for the current module
> auto ModHash = Index.getModuleHash(ModuleID);
> Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
> - for (auto F : ExportList)
> + for (const auto &VI : ExportList) {
> + auto GUID = VI.getGUID();
> // The export list can impact the internalization, be conservative here
> - Hasher.update(ArrayRef<uint8_t>((uint8_t *)&F, sizeof(F)));
> + Hasher.update(ArrayRef<uint8_t>((uint8_t *)&GUID, sizeof(GUID)));
> + }
>
> // Include the hash for every module we import functions from. The set of
> // imported symbols for each module may affect code generation and is
> @@ -383,12 +385,12 @@ static bool isWeakObjectWithRWAccess(GlobalValueSummary *GVS) {
> }
>
> static void thinLTOInternalizeAndPromoteGUID(
> - GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
> - function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
> + GlobalValueSummaryList &GVSummaryList, ValueInfo VI,
> + function_ref<bool(StringRef, ValueInfo)> isExported,
> function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
> isPrevailing) {
> for (auto &S : GVSummaryList) {
> - if (isExported(S->modulePath(), GUID)) {
> + if (isExported(S->modulePath(), VI)) {
> if (GlobalValue::isLocalLinkage(S->linkage()))
> S->setLinkage(GlobalValue::ExternalLinkage);
> } else if (EnableLTOInternalization &&
> @@ -396,7 +398,7 @@ static void thinLTOInternalizeAndPromoteGUID(
> // doesn't resolve them.
> !GlobalValue::isLocalLinkage(S->linkage()) &&
> (!GlobalValue::isInterposableLinkage(S->linkage()) ||
> - isPrevailing(GUID, S.get())) &&
> + isPrevailing(VI.getGUID(), S.get())) &&
> S->linkage() != GlobalValue::AppendingLinkage &&
> // We can't internalize available_externally globals because this
> // can break function pointer equality.
> @@ -415,12 +417,12 @@ static void thinLTOInternalizeAndPromoteGUID(
> // as external and non-exported values as internal.
> void llvm::thinLTOInternalizeAndPromoteInIndex(
> ModuleSummaryIndex &Index,
> - function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
> + function_ref<bool(StringRef, ValueInfo)> isExported,
> function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
> isPrevailing) {
> for (auto &I : Index)
> - thinLTOInternalizeAndPromoteGUID(I.second.SummaryList, I.first, isExported,
> - isPrevailing);
> + thinLTOInternalizeAndPromoteGUID(
> + I.second.SummaryList, Index.getValueInfo(I), isExported, isPrevailing);
> }
>
> // Requires a destructor for std::vector<InputModule>.
> @@ -1330,11 +1332,10 @@ Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
> ExportedGUIDs.insert(
> GlobalValue::getGUID(GlobalValue::dropLLVMManglingEscape(Def)));
>
> - auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
> + auto isExported = [&](StringRef ModuleIdentifier, ValueInfo VI) {
> const auto &ExportList = ExportLists.find(ModuleIdentifier);
> - return (ExportList != ExportLists.end() &&
> - ExportList->second.count(GUID)) ||
> - ExportedGUIDs.count(GUID);
> + return (ExportList != ExportLists.end() && ExportList->second.count(VI)) ||
> + ExportedGUIDs.count(VI.getGUID());
> };
>
> // Update local devirtualized targets that were exported by cross-module
>
> diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
> index 300203bcc1c2..02dda2ab7ac8 100644
> --- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
> +++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
> @@ -589,11 +589,10 @@ struct IsExported {
> const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols)
> : ExportLists(ExportLists), GUIDPreservedSymbols(GUIDPreservedSymbols) {}
>
> - bool operator()(StringRef ModuleIdentifier, GlobalValue::GUID GUID) const {
> + bool operator()(StringRef ModuleIdentifier, ValueInfo VI) const {
> const auto &ExportList = ExportLists.find(ModuleIdentifier);
> - return (ExportList != ExportLists.end() &&
> - ExportList->second.count(GUID)) ||
> - GUIDPreservedSymbols.count(GUID);
> + return (ExportList != ExportLists.end() && ExportList->second.count(VI)) ||
> + GUIDPreservedSymbols.count(VI.getGUID());
> }
> };
>
>
> diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
> index b22cf47a797c..d5419e1a1b9d 100644
> --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
> +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
> @@ -308,7 +308,7 @@ static void computeImportForReferencedGlobals(
>
> 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 @@ static void computeImportForFunction(
> // 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 @@ static void computeImportForFunction(
> // 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 @@ static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index,
> }
> #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 @@ void llvm::ComputeCrossModuleImport(
> 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 c667927d17d1..a00003762ef5 100644
> --- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
> +++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
> @@ -710,7 +710,7 @@ void runWholeProgramDevirtOnIndex(
>
> 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 @@ void updateIndexWPDForExports(
> 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 401be6820439..d795a0a20705 100644
> --- a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
> +++ b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
> @@ -250,12 +250,12 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
> // 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list