[Openmp-commits] [PATCH] D121056: [OpenMP][NFC] Extend the event introduction to allow wait event actions

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert abandoned this revision.
jdoerfert added a comment.

Seems to controversial for now. Will revisit if it comes up later.



================
Comment at: openmp/libomptarget/include/device.h:141
+  /// Returns OFFLOAD_FAIL if something went wrong, OFFLOAD_SUCCESS otherwise.
+  int waitEventIfNecessary(DeviceTy &Device, AsyncInfoTy &AsyncInfo) const;
 
----------------
ye-luo wrote:
> jdoerfert wrote:
> > ye-luo wrote:
> > > both recordEventIfNecessary and waitEventIfNecessary are confusing names "IfNecessary" part.
> > I don't understand what the problem is. Could you say what about it is confusing, maybe suggest an alternative wording or at least describe how that could look like?
> recordEventIfNecessary. It sounds like under certain circumstance defined as necessary, an event will be recorded. Otherwise, it is no-op. What is "necessary"?
> It is not well defined.
> But what the function does is actually create an event if there was no such one and then record that event.
> 
> It can be much cleaner to have 3 APIs.
> createEventIfNonExist. recordEvent, waitEvent. It is not good to blend APIs.
> 
> I also don't get why waitEventIfNecessary is a good API. Should waitEvent just do no-op when there was no event rather than create one?
> It can be much cleaner to have 3 APIs.
> createEventIfNonExist. recordEvent, waitEvent. It is not good to blend APIs.

While there is some truth to this, you simply ignored the "necessary" part in your new "design".
Once you put that in there it is not as clear anymore. createEvent is now potentially creating one, potentially not. That means record and wait have to accept a "null-event" if none was created. This again leads to the question, should they always?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121056