[Openmp-commits] [PATCH] D84996: [OpenMP] Fixed the issue that target memory deallocation might be called when they're being used

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 updated this revision to Diff 282104.
tianshilei1992 added a comment.

Removed the tailing synchronization because it is no use any more


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84996

Files:
  openmp/libomptarget/src/omptarget.cpp


Index: openmp/libomptarget/src/omptarget.cpp
===================================================================
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -421,11 +421,25 @@
   return OFFLOAD_SUCCESS;
 }
 
+namespace {
+/// This structure contains information to deallocate a target pointer
+struct DeallocTgtPtrInfo {
+  void *HstPtrBegin;
+  int64_t DataSize;
+  bool ForceDelete;
+  bool HasCloseModifier;
+
+  DeallocTgtPtrInfo(void *P, int64_t S, bool F, bool H)
+      : HstPtrBegin(P), DataSize(S), ForceDelete(F), HasCloseModifier(H) {}
+};
+} // namespace
+
 /// Internal function to undo the mapping and retrieve the data from the device.
 int targetDataEnd(DeviceTy &Device, int32_t ArgNum, void **ArgBases,
                   void **Args, int64_t *ArgSizes, int64_t *ArgTypes,
                   void **ArgMappers, __tgt_async_info *AsyncInfo) {
   int Ret;
+  std::vector<DeallocTgtPtrInfo> DeallocTgtPtrs;
   // process each input.
   for (int32_t I = ArgNum - 1; I >= 0; --I) {
     // Ignore private variables and arrays - there is no mapping for them.
@@ -574,15 +588,27 @@
       }
       Device.ShadowMtx.unlock();
 
-      // Deallocate map
-      if (DelEntry) {
-        Ret = Device.deallocTgtPtr(HstPtrBegin, DataSize, ForceDelete,
-                                   HasCloseModifier);
-        if (Ret != OFFLOAD_SUCCESS) {
-          DP("Deallocating data from device failed.\n");
-          return OFFLOAD_FAIL;
-        }
-      }
+      // Add pointer to the buffer for later deallocation
+      if (DelEntry)
+        DeallocTgtPtrs.emplace_back(HstPtrBegin, DataSize, ForceDelete,
+                                    HasCloseModifier);
+    }
+  }
+
+  // We need to synchronize before deallocating data
+  Ret = Device.synchronize(AsyncInfo);
+  if (Ret != OFFLOAD_SUCCESS) {
+    DP("Failed to synchronize device.\n");
+    return OFFLOAD_FAIL;
+  }
+
+  // Deallocate target pointer
+  for (DeallocTgtPtrInfo &Info : DeallocTgtPtrs) {
+    Ret = Device.deallocTgtPtr(Info.HstPtrBegin, Info.DataSize,
+                               Info.ForceDelete, Info.HasCloseModifier);
+    if (Ret != OFFLOAD_SUCCESS) {
+      DP("Deallocating data from device failed.\n");
+      return OFFLOAD_FAIL;
     }
   }
 
@@ -1006,5 +1032,5 @@
     return OFFLOAD_FAIL;
   }
 
-  return Device.synchronize(&AsyncInfo);
+  return OFFLOAD_SUCCESS;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84996.282104.patch
Type: text/x-patch
Size: 2413 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20200731/76ef749b/attachment.bin>