-
Notifications
You must be signed in to change notification settings - Fork 15k
[ThinLTO] Avoid creating map entries on lookup (NFCI) #164873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ThinLTO] Avoid creating map entries on lookup (NFCI) #164873
Conversation
We could inadvertently create new entries in the PrevailingModuleForGUID map during lookup, which was always using operator[]. In most cases we will have one for external symbols, but not in cases where the prevailing copy is in a native object. Or if this happened to be looked up for a local. Make the map private and create and use accessors.
|
@llvm/pr-subscribers-lto Author: Teresa Johnson (teresajohnson) ChangesWe could inadvertently create new entries in the PrevailingModuleForGUID Make the map private and create and use accessors. Full diff: https://github.com/llvm/llvm-project/pull/164873.diff 2 Files Affected:
diff --git a/llvm/include/llvm/LTO/LTO.h b/llvm/include/llvm/LTO/LTO.h
index a837cdd95960c..3a4dc5a3dfcf8 100644
--- a/llvm/include/llvm/LTO/LTO.h
+++ b/llvm/include/llvm/LTO/LTO.h
@@ -462,6 +462,19 @@ class LTO {
ModuleMapType ModuleMap;
// The bitcode modules to compile, if specified by the LTO Config.
std::optional<ModuleMapType> ModulesToCompile;
+
+ void setPrevailingModuleForGUID(GlobalValue::GUID GUID, StringRef Module) {
+ PrevailingModuleForGUID[GUID] = Module;
+ }
+ bool isPrevailingModuleForGUID(GlobalValue::GUID GUID,
+ StringRef Module) const {
+ auto It = PrevailingModuleForGUID.find(GUID);
+ return It != PrevailingModuleForGUID.end() && It->second == Module;
+ }
+
+ private:
+ // Make this private so all accesses must go through above accessor methods
+ // to avoid inadvertently creating new entries on lookups.
DenseMap<GlobalValue::GUID, StringRef> PrevailingModuleForGUID;
} ThinLTO;
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 9d0fa116c85bf..b4a21c8c86144 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -1086,15 +1086,15 @@ LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
GlobalValue::getGlobalIdentifier(Sym.getIRName(),
GlobalValue::ExternalLinkage, ""));
if (R.Prevailing)
- ThinLTO.PrevailingModuleForGUID[GUID] = BM.getModuleIdentifier();
+ ThinLTO.setPrevailingModuleForGUID(GUID, BM.getModuleIdentifier());
}
}
if (Error Err =
BM.readSummary(ThinLTO.CombinedIndex, BM.getModuleIdentifier(),
[&](GlobalValue::GUID GUID) {
- return ThinLTO.PrevailingModuleForGUID[GUID] ==
- BM.getModuleIdentifier();
+ return ThinLTO.isPrevailingModuleForGUID(
+ GUID, BM.getModuleIdentifier());
}))
return Err;
LLVM_DEBUG(dbgs() << "Module " << BM.getModuleIdentifier() << "\n");
@@ -1108,8 +1108,8 @@ LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
GlobalValue::getGlobalIdentifier(Sym.getIRName(),
GlobalValue::ExternalLinkage, ""));
if (R.Prevailing) {
- assert(ThinLTO.PrevailingModuleForGUID[GUID] ==
- BM.getModuleIdentifier());
+ assert(
+ ThinLTO.isPrevailingModuleForGUID(GUID, BM.getModuleIdentifier()));
// For linker redefined symbols (via --wrap or --defsym) we want to
// switch the linkage to `weak` to prevent IPOs from happening.
@@ -1988,7 +1988,7 @@ Error LTO::runThinLTO(AddStreamFn AddStream, FileCache Cache,
LocalWPDTargetsMap);
auto isPrevailing = [&](GlobalValue::GUID GUID, const GlobalValueSummary *S) {
- return ThinLTO.PrevailingModuleForGUID[GUID] == S->modulePath();
+ return ThinLTO.isPrevailingModuleForGUID(GUID, S->modulePath());
};
if (EnableMemProfContextDisambiguation) {
MemProfContextDisambiguation ContextDisambiguation;
|
We could inadvertently create new entries in the PrevailingModuleForGUID map during lookup, which was always using operator[]. In most cases we will have one for external symbols, but not in cases where the prevailing copy is in a native object. Or if this happened to be looked up for a local. Make the map private and create and use accessors.
We could inadvertently create new entries in the PrevailingModuleForGUID map during lookup, which was always using operator[]. In most cases we will have one for external symbols, but not in cases where the prevailing copy is in a native object. Or if this happened to be looked up for a local. Make the map private and create and use accessors.
We could inadvertently create new entries in the PrevailingModuleForGUID map during lookup, which was always using operator[]. In most cases we will have one for external symbols, but not in cases where the prevailing copy is in a native object. Or if this happened to be looked up for a local. Make the map private and create and use accessors.
We could inadvertently create new entries in the PrevailingModuleForGUID
map during lookup, which was always using operator[]. In most cases we
will have one for external symbols, but not in cases where the
prevailing copy is in a native object. Or if this happened to be looked
up for a local.
Make the map private and create and use accessors.