[PATCH] D132988: Changes to use a string pool for symbols

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.
Herald added a subscriber: wangpc.

Re-reviewing, since I know you're working on a rebase at the moment. Mostly cosmetic comments, but a handful of functional ones too.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Layer.h:169
 
-private:
+protected:
   ExecutionSession &ES;
----------------
This shouldn't be needed: Clients can access `getExecutionSession` instead.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:297-303
+      auto internedName = G->intern(std::move(S));
+      // todo: wyles revisit this need to rethink the sinking of the name i
+      // guess... urgh...
+      auto ptrCpy = internedName;
+      auto symbol = &G->addExternalSymbol(std::move(internedName), 0, false);
+      symbol->setLive(true);
+      ExternalSymbols[ptrCpy] = symbol;
----------------
Variable names should start with uppercase (according to LLVM coding standards).

You'll have to copy the `SymbolStringPtr` either way, so you can just drop the `move` down to the point where you use it as a map key:
```
auto InternedName = G->intern(S);
auto Symbol = &G->addExternalSymbol(InternedName, 0, false);
Symbol->setLive(true);
ExternalSymbols[std::move(InternedName)] = Symbol;
```


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:358-359
+  for (auto &KeyValue : AlternateNames) {
+    // TODO: wyles comes back and turn the alternateNames into a map of
+    // symbolstringptr
+    auto DefinedSymbolName = G->intern(KeyValue.second);
----------------
This should probably be done before the patch lands.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:364-365
+        ExternalSymbols.count(ExternalSymbolsName)) {
+      auto *Target = DefinedSymbols[DefinedSymbolName];
+      auto *Alias = ExternalSymbols[ExternalSymbolsName];
       G->makeDefined(*Alias, Target->getBlock(), Target->getOffset(),
----------------
You can `std::move` these map keys to save a ref-count. :)


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:376-380
+  llvm::jitlink::Symbol *sym = nullptr;
+  if (!ExternalSymbols.count(SymbolName)) {
+    sym =
+        &G->addExternalSymbol(std::move(SymbolName), Symbol.getValue(), false);
+    ExternalSymbols[sym->getName()] = sym;
----------------
`sym` should be capitalized to `Sym`.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFF_x86_64.cpp:195
+      : SSP(SSP) {
+    imageBaseName = SSP->intern("__ImageBase");
+  }
----------------
`imageBaseName` should be capitalized to `ImageBaseName`.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp:86
       for (auto *Sym : GOTSection->symbols())
-        if (Sym->getName() == ELFGOTSymbolName) {
+        if (Sym->getName() != nullptr && *Sym->getName() == ELFGOTSymbolName) {
           GOTSymbol = Sym;
----------------
What about
```
if (Sym->hasName() *Sym->getName() == ELFGOTSymbolName)
```
?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:300-301
             [&](LinkGraph &LG, Symbol &Sym) -> SectionRangeSymbolDesc {
-              if (Sym.getName() == ELFGOTSymbolName)
+              if (Sym.getName() != nullptr &&
+                  *Sym.getName() == ELFGOTSymbolName)
                 if (auto *GOTSection = G.findSectionByName(
----------------
This could be `if (Sym.hasName() && *Sym.getName() == ELFGOTSymbolName)` too.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:327
       for (auto *Sym : GOTSection->symbols())
-        if (Sym->getName() == ELFGOTSymbolName) {
+        if (Sym->getName() != nullptr && *Sym->getName() == ELFGOTSymbolName) {
           GOTSymbol = Sym;
----------------
`if (Sym.hasName() && *Sym.getName() == ELFGOTSymbolName)` could be used here too.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:203
            "External has already been assigned an address");
-    assert(Sym->getName() != StringRef() && Sym->getName() != "" &&
-           "Externals must be named");
+    assert(Sym->getName() && "Externals must be named");
     SymbolLookupFlags LookupFlags =
----------------
Better yet, `assert(Sym->hasName() && "Externals must be named")`. Sorry I didn't spot that earlier.


================
Comment at: llvm/lib/ExecutionEngine/Orc/ExecutionUtils.cpp:578-579
         jitlink::x86_64::createAnonymousPointer(*G, Sec, &Target);
-    auto NameCopy = G->allocateContent(Twine(getImpPrefix()) + *KV.first);
-    StringRef NameCopyRef = StringRef(NameCopy.data(), NameCopy.size());
-    Ptr.setName(NameCopyRef);
+    auto name = getImpPrefix() + *KV.first;
+    Ptr.setName(G->intern(name.getSingleStringRef()));
     Ptr.setLinkage(jitlink::Linkage::Strong);
----------------
`Twine::getSingleStringRef` is only valid if the twine is actually representing a single StringRef, but here it's a concatenation.

I think you want to replace:

```
auto name = getImpPrefix() + *KV.first;
Ptr.setName(G->intern(name.getSingleStringRef()));
```
with
```
Ptr.setName(G->intern((getImpPrefix() + *KV.first).str()));
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132988/new/

https://reviews.llvm.org/D132988