[OpenMP][libomptarget] Do not rely on AsyncInfoWrapperTy's destructor to synchronize queue

[kevinsala]

Summary

The AsyncInfoWrapperTy's mechanism does not work properly in some cases. I think it's due to copy elision. For instance, the behavior in (A) and (B) is quite different:

Case A:
=======
Error dataSubmit(void *TgtPtr, const void *HstPtr, int64_t Size, __tgt_async_info *AsyncInfo) {
  auto Err = Plugin::success();
  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
  Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
  return Err;
}


Case B:
=======
Error dataSubmit(void *TgtPtr, const void *HstPtr, int64_t Size, __tgt_async_info *AsyncInfo) {
  // This check is added, and it seems it prevents copy elision to be applied.
  if (Size == 0)
    return Plugin::success();

  auto Err = Plugin::success();
  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
  Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
  return Err;
}

In (A), there is no object Error constructed in dataSubmit; it uses an Error object at the caller function directly. Copy elision is applied. Everything works as expected. However, in (B), an object Error is constructed in dataSubmit and then moved to an Error object at the caller side before destroying local variables and returning. The order of actions when returning is the following:

• Err is moved (using move constructor) to a new Error object in the caller function. Err is marked as checked.
• AsyncInfoWrapper is destroyed. It may synchronize the queue/stream, and thus, re-set the Err object. However, Err was already moved, and nobody will check nor return it.
• Destructor of Err fails because the error is not checked.

This patch makes the changes to stop relying on the AsyncInfoWrapperTy destructor to automatically synchronize the stream. Now, the error is provided directly by the wrapper (not a reference anymore) and there is a new function that will perform the synchronization if needed + returning the wrapper's error:

Error dataSubmit(void *TgtPtr, const void *HstPtr, int64_t Size, __tgt_async_info *AsyncInfo) {
  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);

  auto &Err = AsyncInfoWrapper.getError();
  Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
  return AsyncInfoWrapper.finalize();
}

[jdoerfert]

Looks reasonable and clean, @tianshilei1992 @jhuber6 ?

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

281

Unrelated. If this is NFC, just commit this part w/o review.

[tianshilei1992]

Looks reasonable to me.

[kevinsala]

Removed unrelated change

[kevinsala]

8dad7f495302b5f2cd186dcc1cdd76874c80958a

[ye-luo]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

936

I feel bad keeping Err as a member of AsyncInfoWrapper.

dataSubmitImpl sees AsyncInfoWrapper and return Err which actually operates on the member of AsyncInfoWrapper.

second issue if error already happened, finalize attempts a synchronization and overwrites the member Err which is bad for both attempting synchronization and overwriting it when there is already an error.

Can we remove the member Err of AsyncInfoWrapper completely and do
AsyncInfoWrapper.finalize(Err); // if Err is already bad, skip synchronization.
return Err;

[ye-luo]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

936

The above statement

second issue if error already happened, finalize attempts a synchronization and overwrites the member Err which is bad for both attempting synchronization and overwriting it when there is already an error.

It was not correct. The current code already checks Err and performs synchronization only when true.

[kevinsala]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

936

I just opened the patch D148027 to implement this change.

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h

 
 struct GenericPluginTy;
 struct GenericKernelTy;
 struct GenericDeviceTy;
 
 /// Class that wraps the __tgt_async_info to simply its usage. In case the
 /// object is constructed without a valid __tgt_async_info, the object will use
 /// an internal one and will synchronize the current thread with the pending
-/// operations on object destruction.
+/// operations when calling AsyncInfoWrapperTy::finalize(). This latter function
+/// must be called before destroying the wrapper object.
 struct AsyncInfoWrapperTy {
-  AsyncInfoWrapperTy(Error &Err, GenericDeviceTy &Device,
-                     __tgt_async_info *AsyncInfoPtr)
-      : Err(Err), ErrOutParam(&Err), Device(Device),
-        AsyncInfoPtr(AsyncInfoPtr ? AsyncInfoPtr : &LocalAsyncInfo) {}
+  AsyncInfoWrapperTy(GenericDeviceTy &Device, __tgt_async_info *AsyncInfoPtr);
 
-  /// Synchronize with the __tgt_async_info's pending operations if it's the
-  /// internal one.
-  ~AsyncInfoWrapperTy();
+  ~AsyncInfoWrapperTy() {
+    assert(!AsyncInfoPtr && "AsyncInfoWrapperTy not finalized");
+  }
 
   /// Get the raw __tgt_async_info pointer.
   operator __tgt_async_info *() const { return AsyncInfoPtr; }
 
   /// Get a reference to the underlying plugin-specific queue type.
   template <typename Ty> Ty &getQueueAs() const {
     static_assert(sizeof(Ty) == sizeof(AsyncInfoPtr->Queue),
                   "Queue is not of the same size as target type");
     return reinterpret_cast<Ty &>(AsyncInfoPtr->Queue);
   }
 
   /// Indicate whether there is queue.
   bool hasQueue() const { return (AsyncInfoPtr->Queue != nullptr); }
 
+  // Get a reference to the error associated with the asycnhronous operations
+  // related to the async info wrapper.
+  Error &getError() { return Err; }
+
+  /// Synchronize with the __tgt_async_info's pending operations if it's the
+  /// internal async info and return the error associated with the async
+  /// operations. This function must be called before destroying the object.
+  Error finalize();
+
 private:
-  Error &Err;
-  ErrorAsOutParameter ErrOutParam;
+  Error Err;
   GenericDeviceTy &Device;
   __tgt_async_info LocalAsyncInfo;
-  __tgt_async_info *const AsyncInfoPtr;
+  __tgt_async_info *AsyncInfoPtr;
 };
 
 /// Class wrapping a __tgt_device_image and its offload entry table on a
 /// specific device. This class is responsible for storing and managing
 /// the offload entries for an image on a device.
 class DeviceImageTy {
 
   /// Class representing the offload entry table. The class stores the

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

Error init(GenericDeviceTy *Device) {
     this->Device = Device;
     return preallocateDeviceMemory();
   }
 
   void deinit() { Device->free(MemoryStart); }
 
 } RecordReplay;
 
-AsyncInfoWrapperTy::~AsyncInfoWrapperTy() {
+AsyncInfoWrapperTy::AsyncInfoWrapperTy(GenericDeviceTy &Device,
+                                       __tgt_async_info *AsyncInfoPtr)
+    : Err(Plugin::success()), Device(Device),
+      AsyncInfoPtr(AsyncInfoPtr ? AsyncInfoPtr : &LocalAsyncInfo) {
+  // Mark the success as checked. Otherwise, it would produce an error when
+  // re-assigned another error value.
+  !Err;
+}
+
+Error AsyncInfoWrapperTy::finalize() {
+  assert(AsyncInfoPtr && "AsyncInfoWrapperTy already finalized");
+
   // If we used a local async info object we want synchronous behavior.
   // In that case, and assuming the current status code is OK, we will
   // synchronize explicitly when the object is deleted.
   if (AsyncInfoPtr == &LocalAsyncInfo && LocalAsyncInfo.Queue && !Err)
     Err = Device.synchronize(&LocalAsyncInfo);
+
+  // Invalidate the wrapper object.
+  AsyncInfoPtr = nullptr;
+
+  // Return the error associated to the async operations and the synchronize.
+  return std::move(Err);
 }
 
 Error GenericKernelTy::init(GenericDeviceTy &GenericDevice,
                             DeviceImageTy &Image) {
   PreferredNumThreads = getDefaultNumThreads(GenericDevice);
 
   MaxNumThreads = GenericDevice.getThreadLimit();
 
Error GenericKernelTy::launch(GenericDeviceTy &GenericDevice, void **ArgPtrs,
   return launchImpl(GenericDevice, NumThreads, NumBlocks, KernelArgs,
                     KernelArgsPtr, AsyncInfoWrapper);
 }
 
 void *GenericKernelTy::prepareArgs(GenericDeviceTy &GenericDevice,
                                    void **ArgPtrs, ptrdiff_t *ArgOffsets,
                                    int32_t NumArgs,
                                    llvm::SmallVectorImpl<void *> &Args,
                                    llvm::SmallVectorImpl<void *> &Ptrs) const {
   Args.resize(NumArgs);
   Ptrs.resize(NumArgs);
 
   if (NumArgs == 0)
     return nullptr;
 
   for (int I = 0; I < NumArgs; ++I) {
     Ptrs[I] = (void *)((intptr_t)ArgPtrs[I] + ArgOffsets[I]);
if (Kind == TARGET_ALLOC_HOST)
     if (auto Err = PinnedAllocs.unregisterHostBuffer(TgtPtr))
       return Err;
 
   return Plugin::success();
 }
 
 Error GenericDeviceTy::dataSubmit(void *TgtPtr, const void *HstPtr,
                                   int64_t Size, __tgt_async_info *AsyncInfo) {
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
+
+  auto &Err = AsyncInfoWrapper.getError();
   Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::dataRetrieve(void *HstPtr, const void *TgtPtr,
                                     int64_t Size, __tgt_async_info *AsyncInfo) {
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
+
+  auto &Err = AsyncInfoWrapper.getError();
   Err = dataRetrieveImpl(HstPtr, TgtPtr, Size, AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::dataExchange(const void *SrcPtr, GenericDeviceTy &DstDev,
                                     void *DstPtr, int64_t Size,
                                     __tgt_async_info *AsyncInfo) {
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
+
+  auto &Err = AsyncInfoWrapper.getError();
   Err = dataExchangeImpl(SrcPtr, DstDev, DstPtr, Size, AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::launchKernel(void *EntryPtr, void **ArgPtrs,
                                     ptrdiff_t *ArgOffsets,
                                     KernelArgsTy &KernelArgs,
                                     __tgt_async_info *AsyncInfo) {
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
   GenericKernelTy &GenericKernel =
       *reinterpret_cast<GenericKernelTy *>(EntryPtr);
 
   if (RecordReplay.isRecording())
     RecordReplay.saveKernelInputInfo(
         GenericKernel.getName(), ArgPtrs, ArgOffsets, KernelArgs.NumArgs,
         KernelArgs.NumTeams[0], KernelArgs.ThreadLimit[0], KernelArgs.Tripcount,
         AsyncInfoWrapper);
 
+  auto &Err = AsyncInfoWrapper.getError();
   Err = GenericKernel.launch(*this, ArgPtrs, ArgOffsets, KernelArgs,
                              AsyncInfoWrapper);
 
   if (RecordReplay.isRecordingOrReplaying() &&
       RecordReplay.isSaveOutputEnabled())
     RecordReplay.saveKernelOutputInfo(GenericKernel.getName(),
                                       AsyncInfoWrapper);
 
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::initAsyncInfo(__tgt_async_info **AsyncInfoPtr) {
   assert(AsyncInfoPtr && "Invalid async info");
 
   *AsyncInfoPtr = new __tgt_async_info();
 
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, *AsyncInfoPtr);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, *AsyncInfoPtr);
+
+  auto &Err = AsyncInfoWrapper.getError();
   Err = initAsyncInfoImpl(AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::initDeviceInfo(__tgt_device_info *DeviceInfo) {
   assert(DeviceInfo && "Invalid device info");
 
   return initDeviceInfoImpl(DeviceInfo);
 }
 
 Error GenericDeviceTy::printInfo() {
   // TODO: Print generic information here
   return printInfoImpl();
 }
 
 Error GenericDeviceTy::createEvent(void **EventPtrStorage) {
   return createEventImpl(EventPtrStorage);
 }
 
 Error GenericDeviceTy::destroyEvent(void *EventPtr) {
   return destroyEventImpl(EventPtr);
 }
 
 Error GenericDeviceTy::recordEvent(void *EventPtr,
                                    __tgt_async_info *AsyncInfo) {
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
+
+  auto &Err = AsyncInfoWrapper.getError();
   Err = recordEventImpl(EventPtr, AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::waitEvent(void *EventPtr, __tgt_async_info *AsyncInfo) {
-  auto Err = Plugin::success();
-  AsyncInfoWrapperTy AsyncInfoWrapper(Err, *this, AsyncInfo);
+  AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
+
+  auto &Err = AsyncInfoWrapper.getError();
   Err = waitEventImpl(EventPtr, AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
 
 Error GenericDeviceTy::syncEvent(void *EventPtr) {
   return syncEventImpl(EventPtr);
 }
 
 Error GenericPluginTy::init() {
   auto NumDevicesOrErr = initImpl();