[Openmp-commits] [PATCH] D156996: [OpenMP][AMDGPU] Add Envar for controlling HSA busy queue tracking

[Kevin Sala via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1472
     MaxNumQueues = NumHSAQueues;
+    OMPX_QueueTracking = OMPX_QueueTracking.get();
     // Initialize one queue eagerly
----------------
This shouldn't be needed. Envar objects load the value from the environment in their constructor.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1536
     return Plugin::success();
   }
 
----------------
I rewrote the function trying to be a bit simplier. Conceptually, I think the only difference is that `NextQueue` is always increased. Would it make sense?

```
inline Error assignNextQueue(AMDGPUStreamTy *Stream) {
  uint32_t StartIndex = NextQueue % MaxNumQueues;

  if (OMPX_QueueTracking) {
    // Find the first idle queue.
    for (uint32_t I = 0; I < MaxNumQueues; ++I) {
      if (!Queues[StartIndex].isBusy())
        break;

      StartIndex = (StartIndex + 1) % MaxNumQueues;
    }
  }

  // Try to initialize queue and increase its users.
  if (Queues[StartIndex].init(Agent, QueueSize))
    return Err;
  Queues[StartIndex].addUser();

  // Assign the queue.
  Stream->Queue = &Queues[StartIndex];

  // Always move to the next queue.
  ++NextQueue;

  return Plugin::success();
}
```

Also, in the case `OMPX_QueueTracking` is enabled, when no queues are idle, we could fallback to the queue with less users. The minimum can be computed while iterating the queues in that loop.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156996