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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

The logic now looks much more sane to me. This is what I was expecting. Plugins can implement "events" the way they want to.



================
Comment at: openmp/libomptarget/src/device.h:54
+  /// Pointer to the event corresponding to the data movement of this map.
+  mutable std::shared_ptr<std::atomic<void *>> Event;
+
----------------
tianshilei1992 wrote:
> We have to use `shared_ptr` here as this data structure has to be copied but `std::atomic` is un-copyable.
Why is this a shared ptr?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:591
+        while (TPR.MapTableEntry->Event->load() == nullptr)
+          ;
+        int Ret = Device.waitEvent(TPR.MapTableEntry->Event->load(), AsyncInfo);
----------------
I don't get why we need this. At the moment at which we are looking at the Entry I would expect it has an event or not, but neither is cause for waiting. When a new entry is created it should only be exposed to others once the event has been setup as well. Plugins that don't need events can just use nullptr without it being a problem. If this is not a problem, I suspect we want a separate flag in the entry for the status. The event should be allows to be null, the flag can be "in_progress" which indicates the rest needs to wait.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:623
+        }
+      }
       // create shadow pointers for this entry
----------------
Worth a helper to avoid duplication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418