[Openmp-commits] [PATCH] D108528: [OpenMP][Offloading] Add support for event related interfaces

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/include/omptargetplugin.h:160
+// {
+void *__tgt_rtl_create_event(int32_t ID, __tgt_async_info *AsyncInfo);
+
----------------
ye-luo wrote:
> tianshilei1992 wrote:
> > ye-luo wrote:
> > > Please remove AsyncInfo which is not used.
> > I would keep it for compatibility consideration. Note that we are not writing a library only for CUDA. There might be programming models requiring the queue.
> If you think compatibility consideration is real now, please provide details of one case of need. It is not about CUDA, but it is better to avoid hypothetical needs.
> 
> Additional unused arguments adds confusion and cost maintenance.
> 
> With the minimal arguments,
> If a real needs shows up, it is easy to adapt and expand the list arguments.
That's true if the interface is easy to change. This one doesn't seem to be - we carry around legacy entry points in case they're still in use, at least for some period of time. I wonder if we should start putting version numbers in it to establish a path to dropping parts.

To a fair approximation, every C callback interface should take an additional void*. Otherwise you end up with qsort vs qsort_r when inevitably the callback has to do something with state. In this case, the interface is roughly:
```
void * async_info_create();
// do things with the async info
// awkwardly some of these make a new instance if passed 0
void async_info_destroy(void*);
```

Threading a single void* through the sequence of calls gives the plugin a way of associating them with each other. Given multiple host threads and multiple targets that is otherwise extremely difficult, hence every function getting an integer ID.

I would guess that eliding the void* will work fine until we want to have two independent asynchronous regions running on the same target at the same time, at which point we won't know which one to 'record_event' onto.

That's not an argument for this interface in particular, I haven't thought about whether an event should have an identity independent of whatever it is coordinating, but each function in the __tgt_rtl interface that can do things asynchronously should get the async_info instance it is acting on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108528