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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:793
+  /// Indicates that the queue is busy when > 0
+  std::atomic<int> Busy{0};
 };
----------------
rename Busy into sth more meaningful, e.g., `NumUsers`.
Spell out the inc/dec, maybe "addUser", "removeUser"



================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1437
+struct AMDGPUStreamManagerTy
+    : GenericDeviceResourceManagerTy<AMDGPUResourceRef<AMDGPUStreamTy>> {
+  using ResourceRef = AMDGPUResourceRef<AMDGPUStreamTy>;
----------------
Make the class final.


================
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;
----------------
Just resize.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1481
+      }
+    }
+
----------------
This ignores the queue size, doesn't it? We should not grow larger than that value.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1485
+    AMDGPUQueueTy *Queue = nullptr;
+    for (auto &Q : Queues)
+      if (!Q.isBusy()) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154523