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

[Kevin Sala via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:603
+  /// Returns if this queue is considered busy
+  bool isBusy() const { return NumUsers.load() > 0; }
+
----------------
Do we need these three atomic operations with `memory_order_seq_cst`? Or a more relaxed memory order could be enough?


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1476
+      // By default we double the resource pool every time.
+      if (auto Err = ResourcePoolTy::resizeResourcePool(NextAvailable * 2)) {
+        REPORT("Failure to resize the resource pool: %s",
----------------
I'm afraid most of the code in `getResource` and `returnResource` is duplicated from the generic resource manager. Would it be possible to use the original `GenericDeviceResourceManagerTy::getResource()` instead? For instance:

```
ResourceRef getResource() override {
  ResourceRef resource = GenericDeviceResourceManagerTy::getResource();
  // Perform any change on resource
  ...

  return resource;
}
```

(Note that with this modification, the changes on the resource are no longer protected by the stream manager mutex)


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1509
+    uint32_t StartIndex =
+        NextQueue.fetch_add(1, std::memory_order_relaxed) % MaxNumQueues;
+    AMDGPUQueueTy *Q = nullptr;
----------------
I don't think `NextQueue` needs atomic operations if this function is called while holding the stream manager mutex.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1546
+  /// The queues which are assigned to requested streams.
+  std::vector<AMDGPUQueueTy> Queues;
+
----------------
I feel this patch implements a Queue manager inside a Stream manager. Wouldn't it be better to define this logic inside a new `AMDGPUQueueManagerTy` and just have a reference of it in the `AMDGPUStreamManagerTy`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154523