[Openmp-commits] [PATCH] D154312: [Libomptarget] Begin implementing support for RPC services

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:533
+      return Plugin::success();
+    }
+
----------------
This I don't get. We wait for 8k units, then we run the server once, then we wait again if the kernel is not done. Why isn't this part of the loop below with a timeout that adjusts based on the RPC needs?


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1071
+  /// Attach an RPC device to this stream.
+  void setRPCDevice(RPCDeviceTy *Device) { RPCDevice = Device; }
+
----------------
We can grab that information in the constructor.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:558
+  DP("Running an RPC server on device %d\n", getDeviceId());
+  return Plugin::success();
+}
----------------
Can you hoist  Plugin.getRPCServer(), to make it more readable.

I'm not a fan of RPCDevice as a name, we have too many "devices"


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:854
+  /// Boolean indicating whether or not this device has a server attached.
+  RPCDeviceTy *RPCDevice;
 };
----------------
docs


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.cpp:69-104
+  // Register a custom opcode handler to perform plugin specific allocation.
+  // FIXME: We need to make sure this uses asynchronous allocations on CUDA.
+  auto MallocHandler = [](rpc_port_t Port, void *Data) {
+    rpc_recv_and_send(
+        Port,
+        [](rpc_buffer_t *Buffer, void *Data) {
+          plugin::GenericDeviceTy &Device =
----------------
Can the GenericDevice register these handlers?
A vector of them somewhere, or just let it call a register callback.
I would not make these special and define them here.
Move all into the GenericDevice and it will call an Impl method for the subclasses to register more.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.cpp:134
+
+  return &Devices.emplace_back(*this, Device);
+#else
----------------
This seems like a bad idea. We should use the device ID to index the Devices array, and we really need to rename this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154312