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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
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;
 
----------------
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?


================
Comment at: openmp/libomptarget/src/device.cpp:62
+                                             AsyncInfoTy &AsyncInfo) const {
+  return recordOrWaitForEventIfNecessary(*this, Device, AsyncInfo,
+                                         /* Record */ false);
----------------
jdoerfert wrote:
> ye-luo wrote:
> > create  an event without recording it once, waitevent is undefined behavior.
> Is it? I looked at the CUDA API manual and I did not get that idea. Do you happen to have the passage that says so or is it from experimentation? 
I found the concrete word. It is safe.
Before the first call to cudaEventRecord(), an event represents an empty set of work, so for example cudaEventQuery() would return cudaSuccess. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121056