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

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

Dave's suggestion about pass-by-value should be implemented, but otherwise LGTM. Thanks Jared!

There's a part 2 to this bug, which I thought was mentioned in http://llvm.org/PR55533, but I now realize is not: We want to change the interface of `LinkGraph` and associated classes to use `SymbolStringPtr` rather than `StringRef`. That will make it cheap to transfer/copy/compare strings between the current link and the surrounding ORC session, which is something that plugins (and the `ObjectLinkingLayer`) do quite a lot.

Jared -- do you want to tackle that under http://llvm.org/PR55533? Otherwise we can close that (once you land this patch) and file a follow-up issue.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:418
   static Symbol &constructCommon(BumpPtrAllocator &Allocator, Block &Base,
-                                 StringRef Name, orc::ExecutorAddrDiff Size,
-                                 Scope S, bool IsLive) {
-    assert(!Name.empty() && "Common symbol name cannot be empty");
+                                 orc::SymbolStringPtr &&Name,
+                                 orc::ExecutorAddrDiff Size, Scope S,
----------------
dblaikie wrote:
> jaredwy wrote:
> > sgraenitz wrote:
> > > Is there a special reason for these methods to take RValue references? Otherwise, these look like classical sink arguments that should be moved in and taken by value (just remove the `&&`).
> > No not really and in fact started life that way, just was looking to avoid the overhead of the add/decrement of the ownership counter when I noticed every usage was taking ownership, will remove :) 
> If you std::move at the caller and in the callee, you shouldn't need to add/decrement ownership counters even when passing by value. (it'll add an extra move, but no copies - copies are what cause add/decrement), so this should be good
Yep -- the various rvalue reference arguments should all be changed to pass-by-value with the caller using std::move. 


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:954-960
   LinkGraph(std::string Name, const Triple &TT, unsigned PointerSize,
             support::endianness Endianness,
             GetEdgeKindNameFunction GetEdgeKindName)
       : Name(std::move(Name)), TT(TT), PointerSize(PointerSize),
+        Endianness(Endianness), GetEdgeKindName(std::move(GetEdgeKindName)) {
+    SSP = std::make_shared<orc::SymbolStringPool>();
+  }
----------------
What happens if you ditch this overload and require a SymbolStringPool for construction? Does it break many uses (outside the unit tests)?

I guess it's just the `jitlink::link` API (and various implementations of that) that we'd need to update?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132988