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

[Kevin Sala via Phabricator via Openmp-commits]

kevinsala 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:
> 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.
I just opened the patch D148027 to implement this change.


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

https://reviews.llvm.org/D142850