[Openmp-commits] [PATCH] D155903: [Libomptarget] Configure the RPC port count from the plugin

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1660
+      return Err;
+    HardwareParallelism = ComputeUnits * WavesPerCU;
+
----------------
This is valid if a wave opens at most one port at a time. I think an argument could be made that a wave could try to open one port per thread. Likely to be something we should add to the libc tests.

Async also comprises the sizing, though I currently think we should limit openmp to synchronous calls.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:770
+  /// device can support. This is because GPUs in general do not have forward
+  /// progress guaruntees, so we minimize thread level dependencies by
+  /// allocating enough space such that each device thread can have a port. This
----------------
Typos. Also isn't num_teams the wrong constant here? Should be whatever openmp calls warps


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.cpp:64
+      std::min(Device.requestedRPCPortCount(), RPC_MAXIMUM_PORT_COUNT);
+  if (rpc_status_t Err = rpc_server_init(DeviceId, NumPorts,
                                          Device.getWarpSize(), Alloc, &Device))
----------------
This should probably be a hard error if the plugin wants more ports than it can have, especially on platforms where that implies deadlock risk


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:382
+  /// number of ports.
+  uint64_t requestedRPCPortCount() const override {
+    return 2 * NumMuliprocessors;
----------------
It'll affect performance.

Also I'm not totally confident Nvidia has a fair scheduler on SMs, that would be a good thing to check. Amdgpu does not have a fair scheduler on CUs


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:890
+  /// The number of parallel warps that can execute concurrently.
+  uint32_t NumMuliprocessors = 0;
 };
----------------
Typo. Also doesn't seem to match the comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155903