[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:1536
     return Plugin::success();
   }
 
----------------
mhalk wrote:
> 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.
I don't think updating `NextQueue` will have an impact on the tracking mechanism. For the tracking, `NextQueue` just indicates which queue to start looking at. But you will traverse the whole array of queues if we are not finding idle queues.

So, in the first `MaxNumQueues` Stream-request, the mechanism will do the following:

```
1st request: NextQueue is 0 -> check queue 0 (idle) -> break
2nd request: NextQueue is 1 -> check queue 1 (idle) -> break
3rd request: NextQueue is 2 -> check queue 2 (idle) -> break
...
```

In these first `MaxNumQueues` requests, we will always break at the first loop iteration because no one will be using the queue at`NextQueue` positions yet. Then, after those first requests, we will probably start having to iterate over the queues to find if any is idle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156996