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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:936
   Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
-  return Err;
+  return AsyncInfoWrapper.finalize();
 }
----------------
ye-luo wrote:
> 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;
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.


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

https://reviews.llvm.org/D142850