Move a lot of symbol code to use the symbol string pool #115796
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-bolt @llvm/pr-subscribers-backend-risc-v Author: Jared Wyles (jaredwy) ChangesLargely just feeding the Symbol pool through various points it needs to go. This is a first phase to get it in, going to dig in and start to use the pool in further patches. Patch is 154.89 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115796.diff 67 Files Affected:
diff --git a/.gitignore b/.gitignore
index 0e7c6c79001338..a84268a7f68632 100644
--- a/.gitignore
+++ b/.gitignore
@@ -59,6 +59,8 @@ autoconf/autom4te.cache
# VS2017 and VSCode config files.
.vscode
.vs
+#zed config files
+.zed
# pythonenv for github Codespaces
pythonenv*
# clangd index. (".clangd" is a config file now, thus trailing slash)
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/COFF.h b/llvm/include/llvm/ExecutionEngine/JITLink/COFF.h
index 87d3648d37e8ba..33b661933ace83 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/COFF.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/COFF.h
@@ -24,7 +24,8 @@ namespace jitlink {
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromCOFFObject(MemoryBufferRef ObjectBuffer);
+createLinkGraphFromCOFFObject(MemoryBufferRef ObjectBuffer,
+ std::shared_ptr<orc::SymbolStringPool> SSP);
/// Link the given graph.
///
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/COFF_x86_64.h b/llvm/include/llvm/ExecutionEngine/JITLink/COFF_x86_64.h
index fff32d6d960933..2072ae9dfdbe70 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/COFF_x86_64.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/COFF_x86_64.h
@@ -23,8 +23,8 @@ namespace jitlink {
/// Note: The graph does not take ownership of the underlying buffer, nor copy
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
-Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromCOFFObject_x86_64(MemoryBufferRef ObjectBuffer);
+Expected<std::unique_ptr<LinkGraph>> createLinkGraphFromCOFFObject_x86_64(
+ MemoryBufferRef ObjectBuffer, std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be a COFF x86-64 object file.
void link_COFF_x86_64(std::unique_ptr<LinkGraph> G,
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF.h
index 038591f9add055..3decba65f380cb 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF.h
@@ -24,7 +24,8 @@ namespace jitlink {
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject(MemoryBufferRef ObjectBuffer);
+createLinkGraphFromELFObject(MemoryBufferRef ObjectBuffer,
+ std::shared_ptr<orc::SymbolStringPool> SSP);
/// Link the given graph.
///
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch32.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch32.h
index 25d1c3aac2c26e..b865414e520c29 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch32.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch32.h
@@ -24,8 +24,8 @@ namespace jitlink {
/// Note: The graph does not take ownership of the underlying buffer, nor copy
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
-Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_aarch32(MemoryBufferRef ObjectBuffer);
+Expected<std::unique_ptr<LinkGraph>> createLinkGraphFromELFObject_aarch32(
+ MemoryBufferRef ObjectBuffer, std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be an ELF arm/thumb object
/// file.
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h
index 50eb598139ea0b..45a7a0100593f3 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h
@@ -25,8 +25,8 @@ namespace jitlink {
/// Note: The graph does not take ownership of the underlying buffer, nor copy
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
-Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_aarch64(MemoryBufferRef ObjectBuffer);
+Expected<std::unique_ptr<LinkGraph>> createLinkGraphFromELFObject_aarch64(
+ MemoryBufferRef ObjectBuffer, std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be a ELF aarch64 relocatable
/// object file.
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_i386.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_i386.h
index 44ebd969946113..0752f214d9d582 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_i386.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_i386.h
@@ -26,7 +26,8 @@ namespace jitlink {
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_i386(MemoryBufferRef ObjectBuffer);
+createLinkGraphFromELFObject_i386(MemoryBufferRef ObjectBuffer,
+ std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be a ELF i386 relocatable
/// object file.
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_loongarch.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_loongarch.h
index 4d7655c4b988b7..7e5d0f1f918521 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_loongarch.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_loongarch.h
@@ -25,8 +25,8 @@ namespace jitlink {
/// Note: The graph does not take ownership of the underlying buffer, nor copy
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
-Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_loongarch(MemoryBufferRef ObjectBuffer);
+Expected<std::unique_ptr<LinkGraph>> createLinkGraphFromELFObject_loongarch(
+ MemoryBufferRef ObjectBuffer, std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be an ELF loongarch object
/// file.
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h
index 8db986a4a9fa15..c5049a54cdf1d5 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h
@@ -25,15 +25,16 @@ namespace llvm::jitlink {
///
/// WARNING: The big-endian backend has not been tested yet.
Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_ppc64(MemoryBufferRef ObjectBuffer);
+createLinkGraphFromELFObject_ppc64(MemoryBufferRef ObjectBuffer,
+ std::shared_ptr<orc::SymbolStringPool> SSP);
/// Create a LinkGraph from an ELF/ppc64le relocatable object.
///
/// Note: The graph does not take ownership of the underlying buffer, nor copy
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
-Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_ppc64le(MemoryBufferRef ObjectBuffer);
+Expected<std::unique_ptr<LinkGraph>> createLinkGraphFromELFObject_ppc64le(
+ MemoryBufferRef ObjectBuffer, std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be a ELF ppc64le object file.
///
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv.h
index a0e573baca066f..d00b5c2868baf6 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv.h
@@ -26,7 +26,8 @@ namespace jitlink {
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_riscv(MemoryBufferRef ObjectBuffer);
+createLinkGraphFromELFObject_riscv(MemoryBufferRef ObjectBuffer,
+ std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be a ELF riscv object file.
void link_ELF_riscv(std::unique_ptr<LinkGraph> G,
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_x86_64.h b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_x86_64.h
index fbe5765438d240..140dad7b82e002 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/ELF_x86_64.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/ELF_x86_64.h
@@ -24,7 +24,8 @@ namespace jitlink {
/// its contents. The caller is responsible for ensuring that the object buffer
/// outlives the graph.
Expected<std::unique_ptr<LinkGraph>>
-createLinkGraphFromELFObject_x86_64(MemoryBufferRef ObjectBuffer);
+createLinkGraphFromELFObject_x86_64(MemoryBufferRef ObjectBuffe,
+ std::shared_ptr<orc::SymbolStringPool> SSP);
/// jit-link the given object buffer, which must be a ELF x86-64 object file.
void link_ELF_x86_64(std::unique_ptr<LinkGraph> G,
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h b/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
index 9844214c537a06..7d74b622108f9c 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
@@ -23,6 +23,7 @@
#include "llvm/ExecutionEngine/Orc/Shared/ExecutorAddress.h"
#include "llvm/ExecutionEngine/Orc/Shared/ExecutorSymbolDef.h"
#include "llvm/ExecutionEngine/Orc/Shared/MemoryFlags.h"
+#include "llvm/ExecutionEngine/Orc/SymbolStringPool.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/BinaryStreamReader.h"
#include "llvm/Support/BinaryStreamWriter.h"
@@ -35,6 +36,7 @@
#include "llvm/TargetParser/Triple.h"
#include <optional>
+#include <cstring>
#include <map>
#include <string>
#include <system_error>
@@ -422,10 +424,11 @@ class Symbol {
friend class LinkGraph;
private:
- Symbol(Addressable &Base, orc::ExecutorAddrDiff Offset, StringRef Name,
- orc::ExecutorAddrDiff Size, Linkage L, Scope S, bool IsLive,
- bool IsCallable)
- : Name(Name), Base(&Base), Offset(Offset), WeakRef(0), Size(Size) {
+ Symbol(Addressable &Base, orc::ExecutorAddrDiff Offset,
+ orc::SymbolStringPtr &&Name, orc::ExecutorAddrDiff Size, Linkage L,
+ Scope S, bool IsLive, bool IsCallable)
+ : Name(std::move(Name)), Base(&Base), Offset(Offset), WeakRef(0),
+ Size(Size) {
assert(Offset <= MaxOffset && "Offset out of range");
setLinkage(L);
setScope(S);
@@ -434,27 +437,45 @@ class Symbol {
setTargetFlags(TargetFlagsType{});
}
+ static Symbol &constructCommon(BumpPtrAllocator &Allocator, Block &Base,
+ orc::SymbolStringPtr &&Name,
+ orc::ExecutorAddrDiff Size, Scope S,
+ bool IsLive) {
+ assert(Name && "Common symbol name cannot be empty");
+ assert(Base.isDefined() &&
+ "Cannot create common symbol from undefined block");
+ assert(static_cast<Block &>(Base).getSize() == Size &&
+ "Common symbol size should match underlying block size");
+ auto *Sym = Allocator.Allocate<Symbol>();
+ new (Sym)
+ Symbol(Base, 0, std::move(Name), Size, Linkage::Weak, S, IsLive, false);
+ return *Sym;
+ }
+
static Symbol &constructExternal(BumpPtrAllocator &Allocator,
- Addressable &Base, StringRef Name,
+ Addressable &Base,
+ orc::SymbolStringPtr &&Name,
orc::ExecutorAddrDiff Size, Linkage L,
bool WeaklyReferenced) {
assert(!Base.isDefined() &&
"Cannot create external symbol from defined block");
- assert(!Name.empty() && "External symbol name cannot be empty");
+ assert(Name && "External symbol name cannot be empty");
auto *Sym = Allocator.Allocate<Symbol>();
- new (Sym) Symbol(Base, 0, Name, Size, L, Scope::Default, false, false);
+ new (Sym)
+ Symbol(Base, 0, std::move(Name), Size, L, Scope::Default, false, false);
Sym->setWeaklyReferenced(WeaklyReferenced);
return *Sym;
}
static Symbol &constructAbsolute(BumpPtrAllocator &Allocator,
- Addressable &Base, StringRef Name,
+ Addressable &Base,
+ orc::SymbolStringPtr &&Name,
orc::ExecutorAddrDiff Size, Linkage L,
Scope S, bool IsLive) {
assert(!Base.isDefined() &&
"Cannot create absolute symbol from a defined block");
auto *Sym = Allocator.Allocate<Symbol>();
- new (Sym) Symbol(Base, 0, Name, Size, L, S, IsLive, false);
+ new (Sym) Symbol(Base, 0, std::move(Name), Size, L, S, IsLive, false);
return *Sym;
}
@@ -465,20 +486,22 @@ class Symbol {
assert((Offset + Size) <= Base.getSize() &&
"Symbol extends past end of block");
auto *Sym = Allocator.Allocate<Symbol>();
- new (Sym) Symbol(Base, Offset, StringRef(), Size, Linkage::Strong,
- Scope::Local, IsLive, IsCallable);
+ new (Sym) Symbol(Base, Offset, orc::SymbolStringPtr(nullptr), Size,
+ Linkage::Strong, Scope::Local, IsLive, IsCallable);
return *Sym;
}
static Symbol &constructNamedDef(BumpPtrAllocator &Allocator, Block &Base,
- orc::ExecutorAddrDiff Offset, StringRef Name,
+ orc::ExecutorAddrDiff Offset,
+ orc::SymbolStringPtr Name,
orc::ExecutorAddrDiff Size, Linkage L,
Scope S, bool IsLive, bool IsCallable) {
assert((Offset + Size) <= Base.getSize() &&
"Symbol extends past end of block");
- assert(!Name.empty() && "Name cannot be empty");
+ assert(Name && "Name cannot be empty");
auto *Sym = Allocator.Allocate<Symbol>();
- new (Sym) Symbol(Base, Offset, Name, Size, L, S, IsLive, IsCallable);
+ new (Sym)
+ Symbol(Base, Offset, std::move(Name), Size, L, S, IsLive, IsCallable);
return *Sym;
}
@@ -495,18 +518,19 @@ class Symbol {
Symbol &operator=(Symbol &&) = delete;
/// Returns true if this symbol has a name.
- bool hasName() const { return !Name.empty(); }
+ bool hasName() const { return Name != nullptr; }
/// Returns the name of this symbol (empty if the symbol is anonymous).
- StringRef getName() const {
- assert((!Name.empty() || getScope() == Scope::Local) &&
+ const orc::SymbolStringPtr &getName() const {
+ assert((Name || getScope() == Scope::Local) &&
"Anonymous symbol has non-local scope");
+
return Name;
}
/// Rename this symbol. The client is responsible for updating scope and
/// linkage if this name-change requires it.
- void setName(StringRef Name) { this->Name = Name; }
+ void setName(const orc::SymbolStringPtr Name) { this->Name = Name; }
/// Returns true if this Symbol has content (potentially) defined within this
/// object file (i.e. is anything but an external or absolute symbol).
@@ -613,7 +637,7 @@ class Symbol {
/// Set the linkage for this Symbol.
void setLinkage(Linkage L) {
- assert((L == Linkage::Strong || (!Base->isAbsolute() && !Name.empty())) &&
+ assert((L == Linkage::Strong || (!Base->isAbsolute() && Name)) &&
"Linkage can only be applied to defined named symbols");
this->L = static_cast<uint8_t>(L);
}
@@ -623,7 +647,7 @@ class Symbol {
/// Set the visibility for this Symbol.
void setScope(Scope S) {
- assert((!Name.empty() || S == Scope::Local) &&
+ assert((Name || S == Scope::Local) &&
"Can not set anonymous symbol to non-local scope");
assert((S != Scope::Local || Base->isDefined() || Base->isAbsolute()) &&
"Invalid visibility for symbol type");
@@ -675,8 +699,7 @@ class Symbol {
static constexpr uint64_t MaxOffset = (1ULL << 59) - 1;
- // FIXME: A char* or SymbolStringPtr may pack better.
- StringRef Name;
+ orc::SymbolStringPtr Name;
Addressable *Base = nullptr;
uint64_t Offset : 57;
uint64_t L : 1;
@@ -1001,22 +1024,23 @@ class LinkGraph {
using GetEdgeKindNameFunction = const char *(*)(Edge::Kind);
- LinkGraph(std::string Name, const Triple &TT, SubtargetFeatures Features,
- unsigned PointerSize, llvm::endianness Endianness,
+ LinkGraph(std::string Name, std::shared_ptr<orc::SymbolStringPool> SSP,
+ const Triple &TT, SubtargetFeatures Features, unsigned PointerSize,
+ llvm::endianness Endianness,
GetEdgeKindNameFunction GetEdgeKindName)
- : Name(std::move(Name)), TT(TT), Features(std::move(Features)),
+ : Name(std::move(Name)), SSP(SSP), TT(TT), Features(std::move(Features)),
PointerSize(PointerSize), Endianness(Endianness),
GetEdgeKindName(std::move(GetEdgeKindName)) {}
- LinkGraph(std::string Name, const Triple &TT, unsigned PointerSize,
- llvm::endianness Endianness,
+ LinkGraph(std::string Name, std::shared_ptr<orc::SymbolStringPool> SSP,
+ const Triple &TT, unsigned PointerSize, llvm::endianness Endianness,
GetEdgeKindNameFunction GetEdgeKindName)
- : LinkGraph(std::move(Name), TT, SubtargetFeatures(), PointerSize,
+ : LinkGraph(std::move(Name), SSP, TT, SubtargetFeatures(), PointerSize,
Endianness, GetEdgeKindName) {}
- LinkGraph(std::string Name, const Triple &TT,
- GetEdgeKindNameFunction GetEdgeKindName)
- : LinkGraph(std::move(Name), TT, SubtargetFeatures(),
+ LinkGraph(std::string Name, std::shared_ptr<orc::SymbolStringPool> SSP,
+ const Triple &TT, GetEdgeKindNameFunction GetEdgeKindName)
+ : LinkGraph(std::move(Name), SSP, TT, SubtargetFeatures(),
Triple::getArchPointerBitWidth(TT.getArch()) / 8,
TT.isLittleEndian() ? endianness::little : endianness::big,
GetEdgeKindName) {
@@ -1028,6 +1052,7 @@ class LinkGraph {
LinkGraph &operator=(const LinkGraph &) = delete;
LinkGraph(LinkGraph &&) = delete;
LinkGraph &operator=(LinkGraph &&) = delete;
+ ~LinkGraph();
/// Returns the name of this graph (usually the name of the original
/// underlying MemoryBuffer).
@@ -1047,6 +1072,8 @@ class LinkGraph {
const char *getEdgeKindName(Edge::Kind K) const { return GetEdgeKindName(K); }
+ std::shared_ptr<orc::SymbolStringPool> getSymbolStringPool() { return SSP; }
+
/// Allocate a mutable buffer of the given size using the LinkGraph's
/// allocator.
MutableArrayRef<char> allocateBuffer(size_t Size) {
@@ -1260,6 +1287,10 @@ class LinkGraph {
return splitBlockImpl(std::move(Blocks), Cache);
}
+ //
+ orc::SymbolStringPtr intern(StringRef SymbolName) {
+ return SSP->intern(SymbolName);
+ }
/// Add an external symbol.
/// Some formats (e.g. ELF) allow Symbols to have sizes. For Symbols whose
/// size is not known, you should substitute '0'.
@@ -1268,18 +1299,25 @@ class LinkGraph {
/// found or an error will be emitted. Externals that are weakly referenced
/// are permitted to be undefined, in which case they are assigned an address
/// of 0.
- Symbol &addExternalSymbol(StringRef Name, orc::ExecutorAddrDiff Size,
+ Symbol &addExternalSymbol(orc::SymbolStringPtr &&Name,
+ orc::ExecutorAddrDiff Size,
bool IsWeaklyReferenced) {
- assert(!ExternalSymbols.contains(Name) && "Duplicate external symbol");
+ assert(!ExternalSymbols.contains(*Name) && "Duplicate external symbol");
auto &Sym = Symbol::constructExternal(
- Allocator, createAddressable(orc::ExecutorAddr(), false), Name, Size,
- Linkage::Strong, IsWeaklyReferenced);
- ExternalSymbols.insert({Sym.getName(), &Sym});
+ Allocator, createAddressable(orc::ExecutorAddr(), false),
+ std::move(Name), Size, Linkage::Strong, IsWeaklyReferenced);
+ ExternalSymbols.insert({*Sym.getName(), &Sym});
return Sym;
}
+ Symbol &addExternalSymbol(StringRef Name, orc::ExecutorAddrDiff Size,
+ bool IsWeaklyReferenced) {
+ return addExternalSymbol(SSP->intern(Name), Size, IsWeaklyReferenced);
+ }
+
/// Add an absolute symbol.
- Symbol &addAbsoluteSym...
[truncated]
|
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
from
877353c to
c46c0c1
Compare
jaredwy
commented
Nov 14, 2024
| @@ -434,27 +437,45 @@ class Symbol { | |||
| setTargetFlags(TargetFlagsType{}); | |||
| } | |||
|
|
|||
| static Symbol &constructCommon(BumpPtrAllocator &Allocator, Block &Base, | |||
There was a problem hiding this comment.
looks like i added this back in. Ill delete it just let the build bots currently running finish to see if there are any other issues.
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
from
c46c0c1 to
9d304fb
Compare
jaredwy
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo and
dcci
as code owners
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
3 times, most recently
from
bdd2a68 to
3caf856
Compare
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
2 times, most recently
from
5e060d7 to
1aed6c4
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
3 times, most recently
from
b4d5a87 to
d12833c
Compare
lhames
reviewed
Dec 3, 2024
There was a problem hiding this comment.
This is looking great!
I've suggested a bunch of changes, mostly std::move use and other minor stuff. There are just a couple of correctness fixes. I'm ok with this landing as soon as the correctness fixes are done. The cosmetic / perf changes can happen before or after this lands, but we should land it ASAP to avoid bitrot, since this touches a lot of code -- if it'll be a while before you can address all the other comments then let's merge and deal with the other stuff post-commit.
Comment on lines
+62
to
+63
| #zed config files | ||
| .zed |
There was a problem hiding this comment.
Leftover change? (If this is useful we should commit it separately)
bolt/lib/Core/BinaryContext.cpp
Outdated
| @@ -136,7 +137,7 @@ BinaryContext::BinaryContext(std::unique_ptr<MCContext> Ctx, | |||
| std::unique_ptr<MCDisassembler> DisAsm, | |||
| JournalingStreams Logger) | |||
| : Ctx(std::move(Ctx)), DwCtx(std::move(DwCtx)), | |||
| TheTriple(std::move(TheTriple)), TheTarget(TheTarget), | |||
| TheTriple(std::move(TheTriple)), SSP(SSP), TheTarget(TheTarget), | |||
There was a problem hiding this comment.
SSP(std::move(SSP))would eliminate a copy.
bolt/lib/Core/BinaryContext.cpp
Outdated
| @@ -283,7 +285,7 @@ Expected<std::unique_ptr<BinaryContext>> BinaryContext::createBinaryContext( | |||
|
|
|||
| auto BC = std::make_unique<BinaryContext>( | |||
| std::move(Ctx), std::move(DwCtx), std::make_unique<Triple>(TheTriple), | |||
| TheTarget, std::string(TripleName), std::move(MCE), std::move(MOFI), | |||
| SSP, TheTarget, std::string(TripleName), std::move(MCE), std::move(MOFI), | |||
There was a problem hiding this comment.
std::move(SSP)More copy elimination.
bolt/lib/Rewrite/RewriteInstance.cpp
Outdated
| Relocation::Arch = TheTriple.getArch(); | ||
| auto BCOrErr = BinaryContext::createBinaryContext( | ||
| TheTriple, File->getFileName(), Features.get(), IsPIC, | ||
| TheTriple, SSP, File->getFileName(), Features.get(), IsPIC, |
There was a problem hiding this comment.
I wouldn't name this unless it's going to be used below. Instead I'd construct it directly in the argument list:
TheTriple, std::make_shared<orc::SymbolStringPool>(), File->getFileName(), ...
Comment on lines
50
to
53
| std::shared_ptr<orc::SymbolStringPool> ssp = | ||
| std::make_shared<orc::SymbolStringPool>(); | ||
| BC = cantFail(BinaryContext::createBinaryContext( | ||
| ObjFile->makeTriple(), ObjFile->getFileName(), nullptr, true, | ||
| ObjFile->makeTriple(), ssp, ObjFile->getFileName(), nullptr, true, |
There was a problem hiding this comment.
Should be created inline here too.
Comment on lines
75
to
78
| auto SSP = ES.getSymbolStringPool(); | ||
| for (unsigned I = 0; I < 64; I++) { | ||
| auto G = std::make_unique<LinkGraph>("foo", Triple("x86_64-apple-darwin"), | ||
| 8, llvm::endianness::little, | ||
| x86_64::getEdgeKindName); | ||
| auto G = std::make_unique<LinkGraph>( | ||
| "foo", SSP, Triple("x86_64-apple-darwin"), 8, llvm::endianness::little, | ||
| x86_64::getEdgeKindName); |
There was a problem hiding this comment.
Construct pool in argument list.
Comment on lines
141
to
145
|
|
||
| auto G = std::make_unique<LinkGraph>("foo", Triple("x86_64-apple-darwin"), 8, | ||
| llvm::endianness::little, | ||
| x86_64::getEdgeKindName); | ||
| auto SSP = ES.getSymbolStringPool(); | ||
| auto G = std::make_unique<LinkGraph>( | ||
| "foo", SSP, Triple("x86_64-apple-darwin"), 8, llvm::endianness::little, | ||
| getGenericEdgeKindName); |
There was a problem hiding this comment.
Construct pool in argument list.
Comment on lines
195
to
199
|
|
||
| auto G = std::make_unique<LinkGraph>("foo", Triple("x86_64-apple-darwin"), 8, | ||
| llvm::endianness::little, | ||
| x86_64::getEdgeKindName); | ||
| auto SSP = ES.getSymbolStringPool(); | ||
| auto G = std::make_unique<LinkGraph>( | ||
| "foo", SSP, Triple("x86_64-apple-darwin"), 8, llvm::endianness::little, | ||
| getGenericEdgeKindName); |
There was a problem hiding this comment.
Construct pool in argument list.
Comment on lines
250
to
254
| auto G1 = std::make_unique<LinkGraph>("G1", Triple("x86_64-apple-darwin"), | ||
| 8, llvm::endianness::little, | ||
| x86_64::getEdgeKindName); | ||
| auto SSP = ES.getSymbolStringPool(); | ||
| auto G1 = std::make_unique<LinkGraph>( | ||
| "G1", SSP, Triple("x86_64-apple-darwin"), 8, llvm::endianness::little, | ||
| x86_64::getEdgeKindName); |
There was a problem hiding this comment.
Construct pool in argument list.
Comment on lines
268
to
273
| auto G2 = std::make_unique<LinkGraph>("G2", Triple("x86_64-apple-darwin"), | ||
| 8, llvm::endianness::little, | ||
| x86_64::getEdgeKindName); | ||
| auto SSP = ES.getSymbolStringPool(); | ||
| auto G2 = std::make_unique<LinkGraph>( | ||
| "G2", SSP, Triple("x86_64-apple-darwin"), 8, llvm::endianness::little, | ||
| x86_64::getEdgeKindName); |
There was a problem hiding this comment.
Construct pool in argument list.
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
from
4838509 to
2a32d6f
Compare
this reduces the amount of string comparisons we have to do. Mostly a lot of api changes to plump through the string pool instances.
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
from
2a32d6f to
ef47492
Compare
lhames
approved these changes
Dec 4, 2024
Comment on lines
828
to
829
| << CURec->getAddress() << " to point to " | ||
| << (E.getTarget().hasName() ? E.getTarget().getName() | ||
| << formatv("{0:x16}", CURec->getAddress()) << " to point to " |
There was a problem hiding this comment.
Mistaken insertion? ;)
(edit: fixed already -- ignore this)
Comment on lines
47
to
53
|
|
||
| public: | ||
| NormalizedSymbol(const NormalizedSymbol &) = delete; | ||
| NormalizedSymbol &operator=(const NormalizedSymbol &) = delete; | ||
| NormalizedSymbol(NormalizedSymbol &&) = delete; | ||
| NormalizedSymbol &operator=(NormalizedSymbol &&) = delete; | ||
|
|
||
There was a problem hiding this comment.
Whitespace changes should be removed.
| if (LLVM_UNLIKELY(Sym->getName() == ELFTOCSymbolName)) { | ||
| if (LLVM_UNLIKELY(Sym->hasName() && *Sym->getName() == ELFTOCSymbolName)) { |
There was a problem hiding this comment.
Ahh -- it's not interned yet. This is another candidate for interning post-commit.
Comment on lines
-106
to
+110
| if (Sym->getName() == ELFGOTSymbolName) { | ||
| if (*Sym->getName() == ELFGOTSymbolName) { |
There was a problem hiding this comment.
Can this just use (Sym->getName() == InternedGOTSymbolName) ?
|
Oh -- patch needs a |
|
jaredwy
force-pushed
the
wyles/symbolpool-switch
branch
from
0e14270 to
ab63e6a
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/4613 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/7142 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/7214 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/6004 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/80/builds/7248 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/6259 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Largely just feeding the Symbol pool through various points it needs to go. This is a first phase to get it in, going to dig in and start to use the pool in further patches.