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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

A few nits but given the opt-in semantics now I don't see a reason not to include this plugin (as we included other plugins before).



================
Comment at: openmp/docs/SupportAndFAQ.rst:119
+offloading
+<https://openmp.llvm.org/docs/design/Runtimes.html#remote-offloading-plugin>`_.
+By using ``libomptarget.rtl.rpc.so`` and ``openmp-offloading-server``, it is
----------------
I think we can use a generated link here. :ref:`...` maybe with a custom anchor in front of the remote offloading section `.. _remote_offloading:`


================
Comment at: openmp/docs/design/Runtimes.rst:321
+This is the maximum amount of time the client will wait for a response from the server.
+
 LLVM/OpenMP Target Device Runtime (``libomptarget-ARCH-SUBARCH.bc``)
----------------
In the beginning, say that any devices of the remote host will be exposed to the application as if they were local devices. That is you can offload to the host or GPUs of the remote machine using the appropriate device number. If the remote host is the host, you basically see every device twice, once native and through the remote interface.


================
Comment at: openmp/libomptarget/plugins/remote/include/Utils.h:14
+#ifndef INCLUDE_UTILS_H_
+#define INCLUDE_UTILS_H_
+
----------------
Address the clang tidy warning plz


================
Comment at: openmp/libomptarget/plugins/remote/include/Utils.h:47
+/// Helper function to parse common environment variables between client/server
+std::tuple<std::vector<std::string>, uint64_t, uint64_t> parseEnvironment();
+
----------------
Avoid a return type like this and prefer a struct with named members. Also consider passing such a struct as reference to avoid dynamic allocations.


================
Comment at: openmp/libomptarget/plugins/remote/include/Utils.h:97
+void dump(__tgt_device_image *Image);
+
+#endif
----------------
Can we avoid the global namespace here? What if we put everything in the `remoteoffloading` namespace?


================
Comment at: openmp/libomptarget/plugins/remote/lib/Utils.cpp:275
+
+  printf("  %s\n", Line);
+}
----------------
This is magic, add a comment at the top.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95314