[PATCH] D157859: [ORC] Introduce RedirectionManager interface and implementation using JITLink.

[Sunho Kim via Phabricator via llvm-commits]

sunho added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h:59
+                                   ObjectLinkingLayer &ObjLinkingLayer,
+                                   JITDylib &JD, Error &Err)
+      : ES(ES), ObjLinkingLayer(ObjLinkingLayer), JD(JD),
----------------
lhames wrote:
> Can we remove the `JD` argument here? The `ResourceTracker` argument to `createRedirectableSymbols` tells us the target `JITDylib` already, and this would allow us to have one global `JITLinkRediretableSymbolManager`, rather than one per JITDylib.
We need this JD since we are going to generate a stub pool inside this JD. Alternative is manage stub pool per JD within single manager, how does this sound to you?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h:100-101
+
+  BumpPtrAllocator BAlloc;
+  StringSaver StringPool{BAlloc};
+  std::mutex Mutex;
----------------
lhames wrote:
> Why a separate `StringSaver` here, rather than using the `SymbolStringPool`?
We need to get actual string from SymbolStringPtr. Is there a way to do this?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h:32-34
+  virtual Error redirect(SymbolStringPtr Symbol, ExecutorSymbolDef NewDest) {
+    return redirect({{Symbol, NewDest}});
+  }
----------------
lhames wrote:
> I think this should be non-virtual for now. We can make it virtual later if we find a use-case where optimizing the single-symbol case is important.
👍 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157859