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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/include/omptargetplugin.h:160
+// {
+void *__tgt_rtl_create_event(int32_t ID, __tgt_async_info *AsyncInfo);
+
----------------
JonChesterfield wrote:
> jdoerfert wrote:
> > ye-luo wrote:
> > > JonChesterfield wrote:
> > > > 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.
> > > Changing API may never as easy as it might be but this is secondary.
> > > I prefer not adding an argument which is not used. That means it cannot be precisely defined and can be potentially abused. 
> > > For this reason, I'm asking if there was a solid need.
> > > __tgt_rtl_record_event and __tgt_rtl_wait_event should have AsyncInfo. I was only asking for the removal on __tgt_rtl_wait_event __tgt_rtl_sync_event and __tgt_rtl_destroy_event which are not asynchronous APIs.
> > Remove the AsyncInfo in the ones that do not attach the event to a stream/AsyncInfo object.
> What do wait_event or sync_event do? Sounds like they might involve launching a kernel on the GPU that writes to global memory to indicate it has executed, in which case it probably needs an async info.
> 
> Destroy event probably has to wait for some resources on the GPU to no longer be in use, which may be at some distant point in the future, in which case the host probably doesn't want to wait for it.
Changing plugin interfaces will be very painful. All changes since 2019 were all to add new interfaces just to make sure not to break existing applications. If we decide to do it in one way and in the future we ask for another, existing applications will be broken. Of course we can then add another interface such as `__tgt_rtl_create_event_async`, but it's gonna be more confusing. Anyway, I'm fine to remove it, but still more inclined to leave some spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108528