[Openmp-commits] [PATCH] D95314: [OpenMP][Libomptarget] Introduce Remote Offloading Plugin

Mon Jan 25 21:49:26 PST 2021

atmnpatel added a comment.

In D95314#2521637 <https://reviews.llvm.org/D95314#2521637>, @JonChesterfield wrote:

> I don't think any of this is thread safe, which is bad as the plugin can definitely be called from multiple threads on the host.

Yep, it is not designed for that and I added a warning in the documentation.

> The documentation should probably mention that 'printf' will output on the server.

I wrapped it so its only visible during debug/info.

> Given the lack of testing, and some general scepticism about how robustly this handles things going wrong, I think this should be guarded by a cmake variable. LIBOMPTARGET_ENABLE_EXPERIMENTAL_REMOTE_PLUGIN or similar.

Agreed and Done.

> It looks from the valid binary test like this will claim to be a working plugin for most binaries if the server is running. The libomptarget logic for choosing a plugin picks the first plugin that claims to work. I think that will need to be refined to take multiple valid plugins into account, and perhaps surface a user interface for it.

Yessir, ideally we get a config file at runtime that has priorities/a more explicit fallback mechanism.

@tschuett  Thanks for pointing that out! I forgot to check for existing gRPC/protobuf cmake machinery. At this point however, it seems like we'd have to include clang's cmake infrastructure into openmp which seems like a decision that should be made carefully, so I'm going to leave it this way for now.

================
Comment at: openmp/libomptarget/plugins/remote/src/Client.cpp:58
+
+  int Attempts = Shots;
+  while (Attempts--) {
----------------
JonChesterfield wrote:
> This is a bit sketchy. Try again on failure, when the failure cause is unknown, is not very robust. Bounding the number of events by an environment variable seems reasonable though.
> 
> In particular, it means the functions passed must all be safe to run multiple times, as the server might have run it and then fallen over, and that's not clear from the interface.
I removed it for now, it's not necessary for a MVP.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95314