[Openmp-commits] [PATCH] D113119: [OpenMP] Introduce asynchronous malloc and free

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/include/device.h:305
+  /// Flag to indicate if the device supports asynchronous frees.
+  bool SupportsAsyncFree;
+
----------------
ye-luo wrote:
> What about async malloc?
I'll rename.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:321
+
+static CUstream getStream(const int DeviceId, __tgt_async_info *AsyncInfo,
+                          const StreamPoolVectorTy &StreamPoolVector) {
----------------
ye-luo wrote:
> pass StreamPoolTy& as an argument and there is no need of DeviceId
sure.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:371
   public:
-    CUDADeviceAllocatorTy(int DeviceId, std::vector<DeviceDataTy> &DeviceData)
-        : DeviceId(DeviceId), DeviceData(DeviceData) {}
+    CUDADeviceAllocatorTy(int DeviceId, std::vector<DeviceDataTy> &DeviceData,
+                          StreamPoolVectorTy &StreamPoolVector)
----------------
ye-luo wrote:
> This class is one object per device. I would expect removing DeviceId replace vectors with a single element.
That is somewhat orthogonal, a cleanup for all this is following.


================
Comment at: openmp/libomptarget/src/device.cpp:290
+      HostDataToTargetTy::LockGuard LG(*Entry);
+      if (Entry->addEventIfNecessary(*this, AsyncInfo) != OFFLOAD_SUCCESS)
+        return {{false /* IsNewEntry */, false /* IsHostPointer */},
----------------
ye-luo wrote:
> The current Event is intended for D2H, you expanded its usage. In case of one record after alloc and another record after the transfer. I think this will go wrong. Once the event is fulfilled after alloc. other thread may think the transfer has been completed.
Right. 

So checking if we also do D2H would partially help but not completely. The stickiness of the event is still a problem. It might also be for the pure D2H case with multiple transfers as only the first is properly guarded.
I think we need to:
1) only enter it here if we don't do D2H in the same directive (thus below).
2) remove events after we synchronized the AsyncInfo object it was recorded in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113119