[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 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
----------------
JonChesterfield wrote:
> Can async info be an opaque object? One void pointer seems sufficient for arbitrary targets, perhaps with 'queue' renamed to 'state' or similar
We need them both.


================
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);
+
----------------
JonChesterfield wrote:
> 'barrier'? Not sure what dependency means here
That's the interesting part. It is not a barrier. This operation is non-blocking, which means it just inserts the event to the queue, and that's it. So there is no wait.


================
Comment at: openmp/libomptarget/src/device.h:53
+  /// Pointer to the event corresponding to the data movement of this map.
+  volatile mutable void *Event;
+
----------------
JonChesterfield wrote:
> '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?
`mutable` is because `std::set::iterator` is `const`. It makes sense because we don't want to modify the key for the set. However, here `HstPtrBegin` is the key. In order to modify it via the iterator, we need to define it as `mutable`, or use `const_cast` when we want to modify it.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:580
+        // the data movement has not been scheduled yet.
+        while (TPR.MapTableEntry->Event == nullptr)
+          ;
----------------
JonChesterfield wrote:
> 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?
There can still be gap between creating an entry in map table and issuing the data movement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104418