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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 planned changes to this revision.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:24
 #include "Debug.h"
+#include "omptarget.h"
 #include "omptargetplugin.h"
----------------
This should be unrelated...


================
Comment at: openmp/libomptarget/src/device.cpp:14
 #include "device.h"
+#include "omptarget.h"
 #include "private.h"
----------------
must be `clangd` automatic generated code


================
Comment at: openmp/libomptarget/src/device.h:16
 
+#include <atomic>
 #include <cassert>
----------------
unrelated


================
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;
+
----------------
We have to use `shared_ptr` here as this data structure has to be copied but `std::atomic` is un-copyable.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:567
         }
-        // TODO: Attach the event in AsyncInfo to the map table entry if needed
+        // FIXME: We probably need a new flag to indicate whether the device
+        // supports async data movement. If no, we don't have to create the
----------------
Here is very critical. Expect opinions.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:571
+        // Create a new event for this moment
+        void *Event = Device.createEvent(AsyncInfo);
+        // Exchange the old event with new created event
----------------
But with D104555, these whole bunch of code will be moved into `getTargetPointer`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418