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

[Ye Luo via Phabricator via Openmp-commits]

ye-luo requested changes to this revision.
ye-luo added inline comments.
This revision now requires changes to proceed.


================
Comment at: openmp/libomptarget/include/device.h:305
+  /// Flag to indicate if the device supports asynchronous frees.
+  bool SupportsAsyncFree;
+
----------------
What about async malloc?


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


================
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)
----------------
This class is one object per device. I would expect removing DeviceId replace vectors with a single element.


================
Comment at: openmp/libomptarget/src/device.cpp:290
+      HostDataToTargetTy::LockGuard LG(*Entry);
+      if (Entry->addEventIfNecessary(*this, AsyncInfo) != OFFLOAD_SUCCESS)
+        return {{false /* IsNewEntry */, false /* IsHostPointer */},
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113119