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

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

In D104694#2844306 <https://reviews.llvm.org/D104694#2844306>, @sgraenitz wrote:

> Hi Lang

Hi Stefan. Thank you for taking the time to take a look at this, and for the helpful feedback.

> 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.

I think we can require a one-to-one correspondence between TargetProcessControl and ExecutionSession instances. TargetProcessControl authors can handle multiplexing multiple TargetProcessControl instances over one RPC connection if they care to.

>> 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.

"can" is a todo for a follow-up patch.

>> 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.

That would be great if you could. I think you'll have an easier time updating it than I would.



================
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.
----------------
sgraenitz wrote:
> `is` -> `in`?
Good catch -- I'll fix this and update the patch.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcessControl.h:198
+      : TargetProcessControl(SSP ? std::move(SSP)
+                                 : std::make_shared<SymbolStringPool>()) {
+    this->TargetTriple = Triple(TT);
----------------
sgraenitz wrote:
> The null check moved here from the ExecutionSession constructor. Is there a reason why it can't go to the TargetProcessControl constructor right away?
You're right -- I think this would be better done in the TargetProcessControl constructor. I'll fix that when I update the patch.


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


================
Comment at: llvm/tools/lli/lli.cpp:1075
+        ExitOnErr(J->getExecutionSession().getTargetProcessControl().runAsMain(
+            MainSym.getAddress(), InputArgv));
   } else {
----------------
sgraenitz wrote:
> Maybe `auto &TPC = J->getExecutionSession().getTargetProcessControl()` would aid readability here as well?
Yes -- I think that would be an improvement. I'll update that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104694