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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:139
   void *Queue = nullptr;
+  // A pointer to an event-like structure that will be used for tracking
+  // dependences among different asynchronous operations. In CUDA backend, it is
----------------
Can async info be an opaque object? One void pointer seems sufficient for arbitrary targets, perhaps with 'queue' renamed to 'state' or similar


================
Comment at: openmp/libomptarget/include/omptargetplugin.h:148
+// success, return zero. Otherwise, return an error code.
+int32_t __tgt_rtl_set_dependency(int32_t ID, __tgt_async_info *AsyncInfo);
+
----------------
'barrier'? Not sure what dependency means here


================
Comment at: openmp/libomptarget/src/device.h:53
+  /// Pointer to the event corresponding to the data movement of this map.
+  volatile mutable void *Event;
+
----------------
'volatile mutable' reads as 'buggy', why is this volatile or mutable? Also, why is this target thing exposed to the host runtime at all, instead of being purely within an async object?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:580
+        // the data movement has not been scheduled yet.
+        while (TPR.MapTableEntry->Event == nullptr)
+          ;
----------------
Volatile != atomic. Also, I thought the point of this was to do sync on the target, so why is the host busy waiting on something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418