[Openmp-commits] [PATCH] D142850: [OpenMP][libomptarget] Do not rely on AsyncInfoWrapperTy's destructor to synchronize queue

[Kevin Sala via Phabricator via Openmp-commits]

kevinsala created this revision.
kevinsala added reviewers: jdoerfert, jhuber6, tianshilei1992.
kevinsala added a project: OpenMP.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
kevinsala requested review of this revision.
Herald added subscribers: openmp-commits, sstefan1.

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();
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142850

Files:
  openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp
  openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142850.493152.patch
Type: text/x-patch
Size: 9291 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20230129/766e65bd/attachment.bin>