[llvm] [ORC] Reduce `SymbolStringPtr` usage in `InProgressLookupSet` #55576 (PR #116743)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 18 21:38:13 PST 2024
https://github.com/SahilPatidar updated https://github.com/llvm/llvm-project/pull/116743
>From 99d2bac980c8425df155d8344c3851f769209b21 Mon Sep 17 00:00:00 2001
From: SahilPatidar <patidarsahil2001 at gmail.com>
Date: Tue, 19 Nov 2024 10:25:45 +0530
Subject: [PATCH 1/3] [ORC] Reduce `SymbolStringPtr` usage in
`InProgressLookupSet`
---
llvm/include/llvm/ExecutionEngine/Orc/Core.h | 7 +-
.../llvm/ExecutionEngine/Orc/DebugUtils.h | 4 +
llvm/lib/ExecutionEngine/Orc/Core.cpp | 150 ++++++++++++------
llvm/lib/ExecutionEngine/Orc/DebugUtils.cpp | 11 ++
4 files changed, 121 insertions(+), 51 deletions(-)
diff --git a/llvm/include/llvm/ExecutionEngine/Orc/Core.h b/llvm/include/llvm/ExecutionEngine/Orc/Core.h
index e892005c53d8ec..f2b366abaac318 100644
--- a/llvm/include/llvm/ExecutionEngine/Orc/Core.h
+++ b/llvm/include/llvm/ExecutionEngine/Orc/Core.h
@@ -157,6 +157,9 @@ inline JITDylibSearchOrder makeJITDylibSearchOrder(
return O;
}
+using NonOwningSymbolLookupSet =
+ std::vector<std::pair<NonOwningSymbolStringPtr, SymbolLookupFlags>>;
+
/// A set of symbols to look up, each associated with a SymbolLookupFlags
/// value.
///
@@ -1650,8 +1653,8 @@ class ExecutionSession {
/// match a given query from the set of candidate symbols to generate
/// definitions for (no need to generate a definition if one already exists).
Error IL_updateCandidatesFor(JITDylib &JD, JITDylibLookupFlags JDLookupFlags,
- SymbolLookupSet &Candidates,
- SymbolLookupSet *NonCandidates);
+ NonOwningSymbolLookupSet &Candidates,
+ NonOwningSymbolLookupSet *NonCandidates);
/// Handle resumption of a lookup after entering a generator.
void OL_resumeLookupAfterGeneration(InProgressLookupState &IPLS);
diff --git a/llvm/include/llvm/ExecutionEngine/Orc/DebugUtils.h b/llvm/include/llvm/ExecutionEngine/Orc/DebugUtils.h
index 035139578e08f8..392f631b94cbe6 100644
--- a/llvm/include/llvm/ExecutionEngine/Orc/DebugUtils.h
+++ b/llvm/include/llvm/ExecutionEngine/Orc/DebugUtils.h
@@ -82,6 +82,10 @@ raw_ostream &operator<<(raw_ostream &OS, const SymbolLookupSet::value_type &KV);
/// Render a SymbolLookupSet.
raw_ostream &operator<<(raw_ostream &OS, const SymbolLookupSet &LookupSet);
+/// Render a NonOwningSymbolLookupSet.
+raw_ostream &operator<<(raw_ostream &OS,
+ const NonOwningSymbolLookupSet &LookupSet);
+
/// Render a JITDylibSearchOrder.
raw_ostream &operator<<(raw_ostream &OS,
const JITDylibSearchOrder &SearchOrder);
diff --git a/llvm/lib/ExecutionEngine/Orc/Core.cpp b/llvm/lib/ExecutionEngine/Orc/Core.cpp
index 78041993648834..0bd85fd857a6f8 100644
--- a/llvm/lib/ExecutionEngine/Orc/Core.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/Core.cpp
@@ -495,7 +495,10 @@ class InProgressLookupState {
SymbolLookupSet LookupSet, SymbolState RequiredState)
: K(K), SearchOrder(std::move(SearchOrder)),
LookupSet(std::move(LookupSet)), RequiredState(RequiredState) {
- DefGeneratorCandidates = this->LookupSet;
+ for (auto &v : this->LookupSet) {
+ DefGeneratorCandidates.emplace_back(NonOwningSymbolStringPtr(v.first),
+ v.second);
+ }
}
virtual ~InProgressLookupState() = default;
virtual void complete(std::unique_ptr<InProgressLookupState> IPLS) = 0;
@@ -508,8 +511,8 @@ class InProgressLookupState {
size_t CurSearchOrderIndex = 0;
bool NewJITDylib = true;
- SymbolLookupSet DefGeneratorCandidates;
- SymbolLookupSet DefGeneratorNonCandidates;
+ NonOwningSymbolLookupSet DefGeneratorCandidates;
+ NonOwningSymbolLookupSet DefGeneratorNonCandidates;
enum {
NotInGenerator, // Not currently using a generator.
@@ -519,6 +522,14 @@ class InProgressLookupState {
std::vector<std::weak_ptr<DefinitionGenerator>> CurDefGeneratorStack;
};
+static SymbolNameVector getSymbolNames(NonOwningSymbolLookupSet &Symbols) {
+ SymbolNameVector Names;
+ Names.reserve(Symbols.size());
+ for (const auto &KV : Symbols)
+ Names.push_back(SymbolStringPtr(KV.first));
+ return Names;
+}
+
class InProgressLookupFlagsState : public InProgressLookupState {
public:
InProgressLookupFlagsState(
@@ -2220,50 +2231,76 @@ void ExecutionSession::destroyResourceTracker(ResourceTracker &RT) {
Error ExecutionSession::IL_updateCandidatesFor(
JITDylib &JD, JITDylibLookupFlags JDLookupFlags,
- SymbolLookupSet &Candidates, SymbolLookupSet *NonCandidates) {
- return Candidates.forEachWithRemoval(
- [&](const SymbolStringPtr &Name,
- SymbolLookupFlags SymLookupFlags) -> Expected<bool> {
- /// Search for the symbol. If not found then continue without
- /// removal.
- auto SymI = JD.Symbols.find(Name);
- if (SymI == JD.Symbols.end())
- return false;
+ NonOwningSymbolLookupSet &Candidates,
+ NonOwningSymbolLookupSet *NonCandidates) {
+ auto shouldRemove = [&](const NonOwningSymbolStringPtr &Name,
+ SymbolLookupFlags SymLookupFlags) -> Expected<bool> {
+ /// Search for the symbol. If not found then continue without
+ /// removal.
+ auto SymI = JD.Symbols.find(SymbolStringPtr(Name));
+ if (SymI == JD.Symbols.end())
+ return false;
- // If this is a non-exported symbol and we're matching exported
- // symbols only then remove this symbol from the candidates list.
- //
- // If we're tracking non-candidates then add this to the non-candidate
- // list.
- if (!SymI->second.getFlags().isExported() &&
- JDLookupFlags == JITDylibLookupFlags::MatchExportedSymbolsOnly) {
- if (NonCandidates)
- NonCandidates->add(Name, SymLookupFlags);
- return true;
- }
+ // If this is a non-exported symbol and we're matching exported
+ // symbols only then remove this symbol from the candidates list.
+ //
+ // If we're tracking non-candidates then add this to the non-candidate
+ // list.
+ if (!SymI->second.getFlags().isExported() &&
+ JDLookupFlags == JITDylibLookupFlags::MatchExportedSymbolsOnly) {
+ if (NonCandidates)
+ NonCandidates->push_back({Name, SymLookupFlags});
+ return true;
+ }
- // If we match against a materialization-side-effects only symbol
- // then make sure it is weakly-referenced. Otherwise bail out with
- // an error.
- // FIXME: Use a "materialization-side-effects-only symbols must be
- // weakly referenced" specific error here to reduce confusion.
- if (SymI->second.getFlags().hasMaterializationSideEffectsOnly() &&
- SymLookupFlags != SymbolLookupFlags::WeaklyReferencedSymbol)
- return make_error<SymbolsNotFound>(getSymbolStringPool(),
- SymbolNameVector({Name}));
-
- // If we matched against this symbol but it is in the error state
- // then bail out and treat it as a failure to materialize.
- if (SymI->second.getFlags().hasError()) {
- auto FailedSymbolsMap = std::make_shared<SymbolDependenceMap>();
- (*FailedSymbolsMap)[&JD] = {Name};
- return make_error<FailedToMaterialize>(getSymbolStringPool(),
- std::move(FailedSymbolsMap));
- }
+ // If we match against a materialization-side-effects only symbol
+ // then make sure it is weakly-referenced. Otherwise bail out with
+ // an error.
+ // FIXME: Use a "materialization-side-effects-only symbols must be
+ // weakly referenced" specific error here to reduce confusion.
+ if (SymI->second.getFlags().hasMaterializationSideEffectsOnly() &&
+ SymLookupFlags != SymbolLookupFlags::WeaklyReferencedSymbol)
+ return make_error<SymbolsNotFound>(
+ getSymbolStringPool(), SymbolNameVector({SymbolStringPtr(Name)}));
+
+ // If we matched against this symbol but it is in the error state
+ // then bail out and treat it as a failure to materialize.
+ if (SymI->second.getFlags().hasError()) {
+ auto FailedSymbolsMap = std::make_shared<SymbolDependenceMap>();
+ (*FailedSymbolsMap)[&JD] = {SymbolStringPtr(Name)};
+ return make_error<FailedToMaterialize>(getSymbolStringPool(),
+ std::move(FailedSymbolsMap));
+ }
- // Otherwise this is a match. Remove it from the candidate set.
+ // Otherwise this is a match. Remove it from the candidate set.
+ return true;
+ };
+
+ llvm::Error PartitionError = llvm::Error::success();
+
+ // Partitions `Candidates` into those to keep (left) and remove (right) based
+ // on `shouldRemove`. Stores any error in `PartitionError`.
+ auto PartitionPoint = std::partition(
+ Candidates.begin(), Candidates.end(), [&](const auto &Candidate) {
+ const auto &Name = Candidate.first;
+ const auto Flags = Candidate.second;
+
+ auto ShouldRemove = shouldRemove(Name, Flags);
+ if (!ShouldRemove) {
+ PartitionError = ShouldRemove.takeError();
+ return false;
+ }
+ if (*ShouldRemove)
+ return false;
return true;
});
+
+ if (PartitionError)
+ return PartitionError;
+
+ Candidates.erase(PartitionPoint, Candidates.end());
+
+ return Error::success();
}
void ExecutionSession::OL_resumeLookupAfterGeneration(
@@ -2347,9 +2384,11 @@ void ExecutionSession::OL_applyQueryPhase1(
// Add any non-candidates from the last JITDylib (if any) back on to the
// list of definition candidates for this JITDylib, reset definition
// non-candidates to the empty set.
- SymbolLookupSet Tmp;
+ NonOwningSymbolLookupSet Tmp;
std::swap(IPLS->DefGeneratorNonCandidates, Tmp);
- IPLS->DefGeneratorCandidates.append(std::move(Tmp));
+ IPLS->DefGeneratorCandidates.reserve(IPLS->DefGeneratorCandidates.size() + IPLS->DefGeneratorCandidates.size());
+ for (auto &KV : Tmp)
+ IPLS->DefGeneratorCandidates.emplace_back(NonOwningSymbolStringPtr(KV.first), KV.second);
LLVM_DEBUG({
dbgs() << " First time visiting " << JD.getName()
@@ -2443,7 +2482,10 @@ void ExecutionSession::OL_applyQueryPhase1(
{
LLVM_DEBUG(dbgs() << " Attempting to generate " << LookupSet << "\n");
LookupState LS(std::move(IPLS));
- Err = DG->tryToGenerate(LS, K, JD, JDLookupFlags, LookupSet);
+ SymbolLookupSet L;
+ for (auto &V : LookupSet)
+ L.add(SymbolStringPtr(V.first), V.second);
+ Err = DG->tryToGenerate(LS, K, JD, JDLookupFlags, L);
IPLS = std::move(LS.IPLS);
}
@@ -2501,11 +2543,21 @@ void ExecutionSession::OL_applyQueryPhase1(
}
// Remove any weakly referenced candidates that could not be found/generated.
- IPLS->DefGeneratorCandidates.remove_if(
- [](const SymbolStringPtr &Name, SymbolLookupFlags SymLookupFlags) {
- return SymLookupFlags == SymbolLookupFlags::WeaklyReferencedSymbol;
+ auto Pred = [](const NonOwningSymbolStringPtr &Name,
+ SymbolLookupFlags SymLookupFlags) {
+ return SymLookupFlags == SymbolLookupFlags::WeaklyReferencedSymbol;
+ };
+
+ Error PartitionError = Error::success();
+ auto PartitionPoint = std::partition(
+ IPLS->DefGeneratorCandidates.begin(), IPLS->DefGeneratorCandidates.end(),
+ [&](const auto &Candidates) {
+ return !Pred(Candidates.first, Candidates.second);
});
+ IPLS->DefGeneratorCandidates.erase(PartitionPoint,
+ IPLS->DefGeneratorCandidates.end());
+
// If we get here then we've finished searching all JITDylibs.
// If we matched all symbols then move to phase 2, otherwise fail the query
// with a SymbolsNotFound error.
@@ -2515,7 +2567,7 @@ void ExecutionSession::OL_applyQueryPhase1(
} else {
LLVM_DEBUG(dbgs() << "Phase 1 failed with unresolved symbols.\n");
IPLS->fail(make_error<SymbolsNotFound>(
- getSymbolStringPool(), IPLS->DefGeneratorCandidates.getSymbolNames()));
+ getSymbolStringPool(), getSymbolNames(IPLS->DefGeneratorCandidates)));
}
}
diff --git a/llvm/lib/ExecutionEngine/Orc/DebugUtils.cpp b/llvm/lib/ExecutionEngine/Orc/DebugUtils.cpp
index 0f6923a7633f34..ab8a8579849fda 100644
--- a/llvm/lib/ExecutionEngine/Orc/DebugUtils.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/DebugUtils.cpp
@@ -256,6 +256,17 @@ raw_ostream &operator<<(raw_ostream &OS, const SymbolLookupSet &LookupSet) {
PrintAll<SymbolLookupSet::value_type>());
}
+raw_ostream &operator<<(raw_ostream &OS,
+ const NonOwningSymbolLookupSet::value_type &KV) {
+ return OS << "(" << KV.first << ", " << KV.second << ")";
+}
+
+raw_ostream &operator<<(raw_ostream &OS,
+ const NonOwningSymbolLookupSet &LookupSet) {
+ return OS << printSequence(LookupSet, '{', '}',
+ PrintAll<NonOwningSymbolLookupSet::value_type>());
+}
+
raw_ostream &operator<<(raw_ostream &OS,
const JITDylibSearchOrder &SearchOrder) {
OS << "[";
>From 8fd3689f3560597545901440ed8acedcb34aca3f Mon Sep 17 00:00:00 2001
From: SahilPatidar <patidarsahil2001 at gmail.com>
Date: Tue, 19 Nov 2024 10:52:41 +0530
Subject: [PATCH 2/3] Remove FIXME comment
---
llvm/lib/ExecutionEngine/Orc/Core.cpp | 3 ---
1 file changed, 3 deletions(-)
diff --git a/llvm/lib/ExecutionEngine/Orc/Core.cpp b/llvm/lib/ExecutionEngine/Orc/Core.cpp
index 0bd85fd857a6f8..659c165aa12a8b 100644
--- a/llvm/lib/ExecutionEngine/Orc/Core.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/Core.cpp
@@ -488,9 +488,6 @@ Expected<SymbolAliasMap> buildSimpleReexportsAliasMap(JITDylib &SourceJD,
class InProgressLookupState {
public:
- // FIXME: Reduce the number of SymbolStringPtrs here. See
- // https://github.com/llvm/llvm-project/issues/55576.
-
InProgressLookupState(LookupKind K, JITDylibSearchOrder SearchOrder,
SymbolLookupSet LookupSet, SymbolState RequiredState)
: K(K), SearchOrder(std::move(SearchOrder)),
>From 05b2704e567d39c75b6ba6beb9475796008a28ab Mon Sep 17 00:00:00 2001
From: SahilPatidar <patidarsahil2001 at gmail.com>
Date: Tue, 19 Nov 2024 11:06:00 +0530
Subject: [PATCH 3/3] Fix code format
---
llvm/lib/ExecutionEngine/Orc/Core.cpp | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/ExecutionEngine/Orc/Core.cpp b/llvm/lib/ExecutionEngine/Orc/Core.cpp
index 659c165aa12a8b..2a3bf382d41d9a 100644
--- a/llvm/lib/ExecutionEngine/Orc/Core.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/Core.cpp
@@ -2383,9 +2383,11 @@ void ExecutionSession::OL_applyQueryPhase1(
// non-candidates to the empty set.
NonOwningSymbolLookupSet Tmp;
std::swap(IPLS->DefGeneratorNonCandidates, Tmp);
- IPLS->DefGeneratorCandidates.reserve(IPLS->DefGeneratorCandidates.size() + IPLS->DefGeneratorCandidates.size());
+ IPLS->DefGeneratorCandidates.reserve(IPLS->DefGeneratorCandidates.size() +
+ IPLS->DefGeneratorCandidates.size());
for (auto &KV : Tmp)
- IPLS->DefGeneratorCandidates.emplace_back(NonOwningSymbolStringPtr(KV.first), KV.second);
+ IPLS->DefGeneratorCandidates.emplace_back(
+ NonOwningSymbolStringPtr(KV.first), KV.second);
LLVM_DEBUG({
dbgs() << " First time visiting " << JD.getName()
More information about the llvm-commits
mailing list