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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

As a proof of concept, I like this. It shows roughly the plumbing needed to launch a target region on a different machine to the one that is running the main process which clearly has serious potential as a programming model.

Review gets rapidly less detailed after the first few comments. Overall this is a bit weird stylistically but the broad pattern of how the client and server fit together looks as expected.

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.

There are no tests which is unfortunate as there's a lot of subtle control flow in the patch and a lot of ways RPC can go wrong. The documentation should probably mention that 'printf' will output on the server.

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.

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.



================
Comment at: openmp/libomptarget/plugins/remote/include/openmp.proto:42
+
+message Long { int64 number = 1; }
+
----------------
Would probably go with I32 and I64 (or Int32, Int64) instead of Int and Long, as some platforms expect long to be 32 bits


================
Comment at: openmp/libomptarget/plugins/remote/lib/Utils.cpp:69
+    const TargetBinaryDescription *Request, __tgt_bin_desc *Desc,
+    std::map<const void *, __tgt_device_image *> &HostToRemoteDeviceImage) {
+  std::map<const void *, __tgt_offload_entry *> CopiedOffloadEntries;
----------------
std::map on a void* is a little strange - it's an ordered tree structure, so will probably be sorting by address. unordered_map?


================
Comment at: openmp/libomptarget/plugins/remote/lib/Utils.cpp:72
+  (*Desc).NumDeviceImages = Request->images_size();
+  (*Desc).DeviceImages = (__tgt_device_image *)malloc(
+      sizeof(__tgt_device_image) * (*Desc).NumDeviceImages);
----------------
malloc is a bit of a hazard in c++. This looks like it's missing a placement new.

`(*Desc).` to `Desc->` ?


================
Comment at: openmp/libomptarget/plugins/remote/lib/Utils.cpp:83
+
+  // Copy Global Offload Entries
+  __tgt_offload_entry *CurEntry = (*Desc).HostEntriesBegin;
----------------
Not sure about this loop, seems to be at least one too many iterator. Maybe clearer as a `for (size_t i = 0; ...)` style one.


================
Comment at: openmp/libomptarget/plugins/remote/src/Client.cpp:58
+
+  int Attempts = Shots;
+  while (Attempts--) {
----------------
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.


================
Comment at: openmp/libomptarget/plugins/remote/src/rtl.cpp:31
+
+  std::vector<std::string> ServerAddresses = {"0.0.0.0:50051"};
+  if (const char *Env = std::getenv("LIBOMPTARGET_RPC_ADDRESS")) {
----------------
This setup looks similar to some of the code above, perhaps worth putting in a common header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95314