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

[George Rokos via Phabricator via Openmp-commits]

grokos accepted this revision.
grokos added a comment.
This revision is now accepted and ready to land.

A few minor nits. Generally the patch is now in good shape, its logic is much simpler than the first iteration and it's easy to see that we don't risk running into deadlocks or further races. We haven't been very enthusiastic about alternative approaches; meanwhile there's a race waiting to be fixed, so I think we should move on with providing a solution. LGTM on my side, but since a lot of other people have reviewed the patch, let's wait until we hear them, too.



================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:132
 
+void *createEvent(__tgt_async_info *AsyncInfo) {
+  CUstream Stream = reinterpret_cast<CUstream>(AsyncInfo->Queue);
----------------
It's a bit weird that `createEvent` is defined inside the top-level namespace, whereas `waitEvent` and `destroyEvent` are members of `DeviceRTLTy`.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:1379
+
+  int destroyEvent(const int DeviceId, __tgt_async_info *AsyncInfo,
+                   void *EventPtr) const {
----------------
`DeviceId` is not used in this function, so it doesn't need to be an argument.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:592-600
+        // Create a new event for this moment
+        void *Event = Device.createEvent(AsyncInfo);
+        // Exchange the old event with new created event
+        void *OldEvent = Pointer_TPR.MapTableEntry->Event;
+        Pointer_TPR.MapTableEntry->Event = Event;
+        Pointer_TPR.MapTableEntry->unlock();
+        // If the old event is not null, we need to destroy it.
----------------
This code is a direct duplicate of the logic inside `getTargetPointer`. Can we factor it out into a new helper function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418