[OpenMP][libomptarget] Remove error data member from AsyncInfoWrapperTy

[kevinsala]

Summary

This patch removes the Err data member from the AsyncInfoWrapperTy class. Now the error is stored externally, in the caller side, and it is explicitly passed to the AsyncInfoWrapperTy::finalize() function as a reference.

Proposed by @ye-luo in D142850

[jdoerfert]

LG

[ye-luo]

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

208

Could you remove the return?

[kevinsala]

Updated patch removing the error return from AsyncInfoWrapperTy::finalize().

[jhuber6]

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

208

We need to explicitly discard this error now, otherwise it will abort the program when its destructor is called.

[kevinsala]

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

208

But the error now is directly returned by each caller (e.g., GenericDeviceTy::dataSubmit()). Isn't that enough?

[jhuber6]

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

208

Oh, I see this is a reference to an Error presumably someone else handles its destruction? Guess that's allowed. Didn't read it fully, sorry.

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

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();
+  /// internal async info. The error associated to the aysnchronous operations
+  /// issued in this queue must be provided in \p Err. This function will update
+  /// the error parameter with the result of the synchronization if it was
+  /// actually executed. This function must be called before destroying the
+  /// object and only once.
+  void finalize(Error &Err);
 
 private:
-  Error Err;
   GenericDeviceTy &Device;
   __tgt_async_info LocalAsyncInfo;
   __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.

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

public:
   }
 
   void deinit() { Device->free(MemoryStart); }
 
 } RecordReplay;
 
 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.
-  (void)!Err;
-}
+    : Device(Device),
+      AsyncInfoPtr(AsyncInfoPtr ? AsyncInfoPtr : &LocalAsyncInfo) {}
 
-Error AsyncInfoWrapperTy::finalize() {
+void AsyncInfoWrapperTy::finalize(Error &Err) {
   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 we used a local async info object we want synchronous behavior. In that
+  // case, and assuming the current status code is correct, we will synchronize
+  // explicitly when the object is deleted. Update the error with the result of
+  // the synchronize operation.
   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 GenericDeviceTy::dataDelete(void *TgtPtr, TargetAllocTy Kind) {
 
   return Plugin::success();
 }
 
 Error GenericDeviceTy::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();
+  auto Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::dataRetrieve(void *HstPtr, const void *TgtPtr,
                                     int64_t Size, __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
-  auto &Err = AsyncInfoWrapper.getError();
-  Err = dataRetrieveImpl(HstPtr, TgtPtr, Size, AsyncInfoWrapper);
-  return AsyncInfoWrapper.finalize();
+  auto Err = dataRetrieveImpl(HstPtr, TgtPtr, Size, AsyncInfoWrapper);
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::dataExchange(const void *SrcPtr, GenericDeviceTy &DstDev,
                                     void *DstPtr, int64_t Size,
                                     __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
-  auto &Err = AsyncInfoWrapper.getError();
-  Err = dataExchangeImpl(SrcPtr, DstDev, DstPtr, Size, AsyncInfoWrapper);
-  return AsyncInfoWrapper.finalize();
+  auto Err = dataExchangeImpl(SrcPtr, DstDev, DstPtr, Size, AsyncInfoWrapper);
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::launchKernel(void *EntryPtr, void **ArgPtrs,
                                     ptrdiff_t *ArgOffsets,
                                     KernelArgsTy &KernelArgs,
                                     __tgt_async_info *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,
+  auto Err = GenericKernel.launch(*this, ArgPtrs, ArgOffsets, KernelArgs,
                                   AsyncInfoWrapper);
 
   if (RecordReplay.isRecordingOrReplaying() &&
       RecordReplay.isSaveOutputEnabled())
     RecordReplay.saveKernelOutputInfo(GenericKernel.getName(),
                                       AsyncInfoWrapper);
 
-  return AsyncInfoWrapper.finalize();
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::initAsyncInfo(__tgt_async_info **AsyncInfoPtr) {
   assert(AsyncInfoPtr && "Invalid async info");
 
   *AsyncInfoPtr = new __tgt_async_info();
 
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, *AsyncInfoPtr);
 
-  auto &Err = AsyncInfoWrapper.getError();
-  Err = initAsyncInfoImpl(AsyncInfoWrapper);
-  return AsyncInfoWrapper.finalize();
+  auto Err = initAsyncInfoImpl(AsyncInfoWrapper);
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::initDeviceInfo(__tgt_device_info *DeviceInfo) {
   assert(DeviceInfo && "Invalid device info");
 
   return initDeviceInfoImpl(DeviceInfo);
 }
 

 Error GenericDeviceTy::destroyEvent(void *EventPtr) {
   return destroyEventImpl(EventPtr);
 }
 
 Error GenericDeviceTy::recordEvent(void *EventPtr,
                                    __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
-  auto &Err = AsyncInfoWrapper.getError();
-  Err = recordEventImpl(EventPtr, AsyncInfoWrapper);
-  return AsyncInfoWrapper.finalize();
+  auto Err = recordEventImpl(EventPtr, AsyncInfoWrapper);
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::waitEvent(void *EventPtr, __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
-  auto &Err = AsyncInfoWrapper.getError();
-  Err = waitEventImpl(EventPtr, AsyncInfoWrapper);
-  return AsyncInfoWrapper.finalize();
+  auto Err = waitEventImpl(EventPtr, AsyncInfoWrapper);
+  AsyncInfoWrapper.finalize(Err);
+  return Err;
 }
 
 Error GenericDeviceTy::syncEvent(void *EventPtr) {
   return syncEventImpl(EventPtr);
 }
 
 Error GenericPluginTy::init() {
   auto NumDevicesOrErr = initImpl();