[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:1503
+  /// Get stream from the pool or create new resources.
+  virtual Error getResource(AMDGPUStreamTy *&TargetReference) override {
+    return getResourcesImpl(1, &TargetReference, [this](AMDGPUStreamTy *&Ref) {
----------------
I would rename the parameter to `Resource`, `Handle`, or similar. The parameter is no longer a `ResourceReference`. The same for the lambda's parmeter and the `returnResource` function.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1505
+    return getResourcesImpl(1, &TargetReference, [this](AMDGPUStreamTy *&Ref) {
+      assignNextQueue(Ref);
+      return Plugin::success();
----------------
You can make this function to return the error and propagate it outside. Then, the `assignNextQueue` function can return the error when failing to initialize a queue, instead of handling it.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1513
+    return returnResourceImpl(Reference, [](AMDGPUStreamTy *Ref) {
+      (*Ref).Queue->removeUser();
+      return Plugin::success();
----------------



================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1521
+  /// there is no queue without current users, resort to round robin selection.
+  inline void assignNextQueue(ResourceRef Reference) {
+    uint32_t StartIndex = NextQueue % MaxNumQueues;
----------------
You can work with a specific parameter `AMDGPUStreamTy *Stream` directly.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1535
+          REPORT("Failure during queue init: %s\n",
+                 toString(std::move(Err)).data());
+          Q = &Queues[0];
----------------
```
if (auto Err = Q->initLazy(Agent, QueueSize))
  return Err;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154523