[PATCH] D104694: [ORC] Require TargetProcessControl instance when constructing ExecutionSession.

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Hi Lang

I think ES taking ownership of TPC makes a lot of sense. Sessions usually have a unique target process. I am not yet sure how it affects RPC scenarios, where clients might want to run multiple sessions subsequently or in parallel over one RPC connection. Separating the RPC parts from the TPC parts in OrcRPCTargetProcessControlBase? I didn't look into it, but it doesn't sound too bad on the first glance.

> Simplify termination, since TargetProcessControl::disconnect can now be called automatically from ExecutionSession::endSession.

Does "can" mean it's a todo for a follow-up patch or was it supposed to be part of this review? ExecutionSession::endSession() hasn't changed (yet) and llvm-jitlink still calls TargetProcessControl::disconnect() manually.

> This patch updates all in-tree code and examples except for LLJITWithRemoteDebugging

I will disable the example build+test for the moment and adjust+re-enable it once this patch landed.

+ a few minor details inline



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcessControl.h:190
+/// A TargetProcessControl instance that asserts if any of its methods are used.
+/// Suitable for use is unit tests, and by ORC clients who haven't moved to
+/// TargetProcessControl-based APIs yet.
----------------
`is` -> `in`?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcessControl.h:198
+      : TargetProcessControl(SSP ? std::move(SSP)
+                                 : std::make_shared<SymbolStringPool>()) {
+    this->TargetTriple = Triple(TT);
----------------
The null check moved here from the ExecutionSession constructor. Is there a reason why it can't go to the TargetProcessControl constructor right away?


================
Comment at: llvm/lib/ExecutionEngine/Orc/TargetProcessControl.cpp:49
+    SSP = std::make_shared<SymbolStringPool>();
+
   auto PageSize = sys::Process::getPageSize();
----------------
Another null check here. Add one more in OrcRPCTargetProcessControlBase?


================
Comment at: llvm/tools/lli/lli.cpp:1075
+        ExitOnErr(J->getExecutionSession().getTargetProcessControl().runAsMain(
+            MainSym.getAddress(), InputArgv));
   } else {
----------------
Maybe `auto &TPC = J->getExecutionSession().getTargetProcessControl()` would aid readability here as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104694