[OpenMP][NFC] Extend the event introduction to allow wait event actions

[jdoerfert]

Summary

Similar to the (old) addEventIfNecessary we now allow to add an event
and wait for it if that is necessary. This allows a thread to put in
a wait before the event was created yet. Since this is not yet used this
patch won't change anything.

[ye-luo]

openmp/libomptarget/include/device.h

141

both recordEventIfNecessary and waitEventIfNecessary are confusing names "IfNecessary" part.

openmp/libomptarget/src/device.cpp

46

Cannot take the logic of Record

62

create an event without recording it once, waitevent is undefined behavior.

[jdoerfert]

openmp/libomptarget/include/device.h

141

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?

openmp/libomptarget/src/device.cpp

46

I do not understand what you mean here either.

62

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?

[ye-luo]

openmp/libomptarget/include/device.h

141

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?

openmp/libomptarget/src/device.cpp

62

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.

[jdoerfert]

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

openmp/libomptarget/include/device.h

141

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?

openmp/libomptarget/include/device.h

public:
   uint64_t getDynRefCount() const { return States->DynRefCount; }
 
   /// Get the hold reference count.
   uint64_t getHoldRefCount() const { return States->HoldRefCount; }
 
   /// Get the event bound to this data map.
   void *getEvent() const { return States->Event; }
 
-  /// Add a new event, if necessary.
+  /// Add a new event, if necessary, and record it.
   /// Returns OFFLOAD_FAIL if something went wrong, OFFLOAD_SUCCESS otherwise.
-  int addEventIfNecessary(DeviceTy &Device, AsyncInfoTy &AsyncInfo) const;
+  int recordEventIfNecessary(DeviceTy &Device, AsyncInfoTy &AsyncInfo) const;
+
+  /// Add a new event, if necessary, and wait for it.
+  /// Returns OFFLOAD_FAIL if something went wrong, OFFLOAD_SUCCESS otherwise.
+  int waitEventIfNecessary(DeviceTy &Device, AsyncInfoTy &AsyncInfo) const;
 
   /// Set the event bound to this data map.
   void setEvent(void *Event) const { States->Event = Event; }
 
   /// Reset the specified reference count unless it's infinity.  Reset to 1
   /// (even if currently 0) so it can be followed by a decrement.
   void resetRefCount(bool UseHoldRefCount) const {
     uint64_t &ThisRefCount =

openmp/libomptarget/src/device.cpp

 #include "private.h"
 #include "rtl.h"
 
 #include <cassert>
 #include <climits>
 #include <cstdio>
 #include <string>
 
-int HostDataToTargetTy::addEventIfNecessary(DeviceTy &Device,
-                                            AsyncInfoTy &AsyncInfo) const {
+static int recordOrWaitForEventIfNecessary(const HostDataToTargetTy &HDTT,
+                                           DeviceTy &Device,
+                                           AsyncInfoTy &AsyncInfo,
+                                           bool Record) {
   // First, check if the user disabled atomic map transfer/malloc/dealloc.
   if (!PM->UseEventsForAtomicTransfers)
     return OFFLOAD_SUCCESS;
 
-  void *Event = getEvent();
+  void *Event = HDTT.getEvent();
   bool NeedNewEvent = Event == nullptr;
   if (NeedNewEvent && Device.createEvent(&Event) != OFFLOAD_SUCCESS) {
     REPORT("Failed to create event\n");
     return OFFLOAD_FAIL;
   }
 
   // We cannot assume the event should not be nullptr because we don't
   // know if the target support event. But if a target doesn't,
-  // recordEvent should always return success.
-  if (Device.recordEvent(Event, AsyncInfo) != OFFLOAD_SUCCESS) {
-    REPORT("Failed to set dependence on event " DPxMOD "\n", DPxPTR(Event));
+  // recordEvent and waitEvent should always return success.
+  if (Record && Device.recordEvent(Event, AsyncInfo) != OFFLOAD_SUCCESS) {
+    REPORT("Failed to record event " DPxMOD "\n", DPxPTR(Event));
+    return OFFLOAD_FAIL;
+  }
+  if (!Record && Device.waitEvent(Event, AsyncInfo) != OFFLOAD_SUCCESS) {
+    REPORT("Failed to wait on event " DPxMOD "\n", DPxPTR(Event));
     return OFFLOAD_FAIL;
   }
 
   if (NeedNewEvent)
-    setEvent(Event);
+    HDTT.setEvent(Event);
 
   return OFFLOAD_SUCCESS;
 }
+int HostDataToTargetTy::recordEventIfNecessary(DeviceTy &Device,
+                                               AsyncInfoTy &AsyncInfo) const {
+  return recordOrWaitForEventIfNecessary(*this, Device, AsyncInfo,
+                                         /* Record */ true);
+}
+int HostDataToTargetTy::waitEventIfNecessary(DeviceTy &Device,
+                                             AsyncInfoTy &AsyncInfo) const {
+  return recordOrWaitForEventIfNecessary(*this, Device, AsyncInfo,
+                                         /* Record */ false);
+}
 
 DeviceTy::DeviceTy(RTLInfoTy *RTL)
     : DeviceID(-1), RTL(RTL), RTLDeviceID(-1), IsInit(false), InitFlag(),
       HasPendingGlobals(false), HostDataToTargetMap(), PendingCtorsDtors(),
       ShadowPtrMap(), DataMapMtx(), PendingGlobalsMtx(), ShadowMtx() {}
 
 DeviceTy::~DeviceTy() {
   if (DeviceID == -1 || !(getInfoLevel() & OMP_INFOTYPE_DUMP_TABLE))
if (TargetPointer && !IsHostPtr && HasFlagTo && (IsNew || HasFlagAlways)) {
 
     int Ret = submitData(TargetPointer, HstPtrBegin, Size, AsyncInfo);
     if (Ret != OFFLOAD_SUCCESS) {
       REPORT("Copying data to device failed.\n");
       // We will also return nullptr if the data movement fails because that
       // pointer points to a corrupted memory region so it doesn't make any
       // sense to continue to use it.
       TargetPointer = nullptr;
-    } else if (Entry->addEventIfNecessary(*this, AsyncInfo) != OFFLOAD_SUCCESS)
+    } else if (Entry->recordEventIfNecessary(*this, AsyncInfo) !=
+               OFFLOAD_SUCCESS)
       return {{false /* IsNewEntry */, false /* IsHostPointer */},
               {} /* MapTableEntry */,
               nullptr /* TargetPointer */};
   } else {
     // Release the mapping table lock directly.
     DataMapMtx.unlock();
     // If not a host pointer and no present modifier, we need to wait for the
     // event if it exists.

openmp/libomptarget/src/omptarget.cpp

if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ && !IsHostPtr) {
         TgtPtrBase = ExpectedTgtPtrBase;
 
         int Ret = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
                                     sizeof(void *), AsyncInfo);
         if (Ret != OFFLOAD_SUCCESS) {
           REPORT("Copying data to device failed.\n");
           return OFFLOAD_FAIL;
         }
-        if (Pointer_TPR.MapTableEntry->addEventIfNecessary(Device, AsyncInfo) !=
-            OFFLOAD_SUCCESS)
+        if (Pointer_TPR.MapTableEntry->recordEventIfNecessary(
+                Device, AsyncInfo) != OFFLOAD_SUCCESS)
           return OFFLOAD_FAIL;
       } else
         Device.ShadowMtx.unlock();
     }
   }
 
   return OFFLOAD_SUCCESS;
 }