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

Johannes Doerfert via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Thu Jan 20 10:33:39 PST 2022

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

Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:371
-    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.

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.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list