[Openmp-commits] [PATCH] D154523: [OpenMP][AMDGPU] Tracking of busy HSA queues

[Michael Halkenhäuser via Phabricator via Openmp-commits]

mhalk added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1445
+  Error init(uint32_t InitialSize, int NumHsaQueues, int HsaQueueSize) {
+    Queues = std::vector<AMDGPUQueueTy>(NumHsaQueues);
+    QueueSize = HsaQueueSize;
----------------
jdoerfert wrote:
> Just resize.
Initially, I wanted to do that, but `AMDGPUQueueTy` has a member (Mutex) with deleted copy ctor.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1448
+    // Initialize one queue eagerly.
+    if (auto Err = Queues.front().init(Agent, QueueSize))
+      REPORT("Failure during first queue init: %s\n",
----------------
jplehr wrote:
> In case of error, I think we should report and return the error instead of reporting and keep going.
> 
> This is the initial queue, so if this fails, there is no queue available.
Agreed; as discussed, we'll return `Err` and not report it at this level.

It will be reported by the higher layer, for example like this:
//"PluginInterface" error: Failure to initialize device 0: Error in hsa_queue_create: HSA_STATUS_ERROR_INVALID_ARGUMENT: One of the actual arguments does not meet a precondition stated in the documentation of the corresponding formal argument.//


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1481
+      }
+    }
+
----------------
jdoerfert wrote:
> This ignores the queue size, doesn't it? We should not grow larger than that value.
To the best of my knowledge we should not exceed the set (max) number of queues.
This will only resize the streams, as the size of `Queues` is only set once, during `init(...)`.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1485
+    AMDGPUQueueTy *Queue = nullptr;
+    for (auto &Q : Queues)
+      if (!Q.isBusy()) {
----------------
jdoerfert wrote:
> Start with `Current = NextQueue.fetch_add(1, std::memory_order_relaxed) % Size` when you iterate, that way you won't check the first queue all the time.
Agreed; as discussed, we're going with two loops to search for an idle queue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154523