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

[Lang Hames via Phabricator via llvm-commits]

lhames 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),
----------------
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.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h:100-101
+
+  BumpPtrAllocator BAlloc;
+  StringSaver StringPool{BAlloc};
+  std::mutex Mutex;
----------------
Why a separate `StringSaver` here, rather than using the `SymbolStringPool`?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h:32-34
+  virtual Error redirect(SymbolStringPtr Symbol, ExecutorSymbolDef NewDest) {
+    return redirect({{Symbol, NewDest}});
+  }
----------------
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.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h:54
+  /// desitnation symbol.
+  virtual Error createRedirectableSymbol(SymbolStringPtr Symbol,
+                                         ExecutorSymbolDef InitialDest,
----------------
Similarly here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157859