[Openmp-commits] [PATCH] D104418: [OpenMP][Offloading] Fixed data race in libomptarget caused by async data movement

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked 2 inline comments as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/device.cpp:276
 
-    int Ret = submitData(TargetPointer, HstPtrBegin, Size, AsyncInfo);
+      int Ret = submitData(TargetPointer, HstPtrBegin, Size, AsyncInfo);
 
----------------
ye-luo wrote:
> I think the purpose of introducing this data transfer event is to achieve the atomic transfer behavior.
> So it is unsafe to issue transfer right away without checking the status of Event.
> If a map(always comes from a different thread.
The event has to be created after the issue. At that moment, the entry might not have any associated event.


================
Comment at: openmp/libomptarget/src/device.cpp:295
+      Entry->unlock();
+      // If there is an event attached, destroy it.
+      if (OldEvent)
----------------
ye-luo wrote:
> The current event never got destroyed unitl the next transfer.
> 1. event doesn't got properly destroyed.
> 2. almost like one event per map.
For your first comment, yes, and now it is fixed by an ugly way. A nicer solution would be related to the API design in D108528where which we can discuss. The question is whether we want to pass async info to the plugin for event creation, synchronization, and destroy.

For the second point, it's a compatibility consideration, that is if there is a platform that doesn't allow event to be reused after it is fulfilled. I don't know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418