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

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

mhalk 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
----------------
kevinsala wrote:
> This shouldn't be needed. Envar objects load the value from the environment in their constructor.
Good catch, thanks! I wasn't aware of that.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1536
     return Plugin::success();
   }
 
----------------
kevinsala wrote:
> 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.
> 
Really like the reduced complexity.

Now that `NextQueue` is always increased wouldn't that forfeit the purpose of handling busy queues? -- at least for the first `MaxNumQueues` Stream-requests.
Because I think we will now always look at a Queue that has not been initialized and is therefore considered "idle"/not busy.
I guess keeping `NextQueue` at zero until the maximum number of queues is actually initialized is important, unless we find sth. equivalent.
I'll think about it a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156996