[Openmp-commits] [PATCH] D105121: [OpenMP] Avoid checking parent reference count in targetDataBegin

[Joel E. Denny via Phabricator via Openmp-commits]

jdenny created this revision.
jdenny added reviewers: grokos, RaviNarayanaswamy, jdoerfert, jhuber6.
Herald added subscribers: guansong, yaxunl.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added a project: OpenMP.

This patch is an attempt to do for `targetDataBegin` what D104924 <https://reviews.llvm.org/D104924> does
for `targetDataEnd`:

- Eliminates a lock/unlock of the data mapping table.
- Clarifies the logic that determines whether a struct member's host-to-device transfer occurs.  The old logic, which checks the parent struct's reference count, is a leftover from back when we had a different map interface (as pointed out at https://reviews.llvm.org/D104924#2846972).

Additionally, it eliminates the `DeviceTy::getMapEntryRefCnt`, which
is no longer used after this patch.

While D104924 <https://reviews.llvm.org/D104924> does not change the computation of `IsLast`, I found I
needed to change the computation of `IsNew` for this patch.  As far as
I can tell, the change is correct, and this patch does not cause any 
additional `openmp` tests to fail.  However, I'm not sure I've thought
of all use cases.  Please advise.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105121

Files:
  openmp/libomptarget/src/device.cpp
  openmp/libomptarget/src/device.h
  openmp/libomptarget/src/omptarget.cpp


Index: openmp/libomptarget/src/omptarget.cpp
===================================================================
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -536,20 +536,8 @@
       bool copy = false;
       if (!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
           HasCloseModifier) {
-        if (IsNew || (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)) {
+        if (IsNew || (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS))
           copy = true;
-        } else if ((arg_types[i] & OMP_TGT_MAPTYPE_MEMBER_OF) &&
-                   !(arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ)) {
-          // Copy data only if the "parent" struct has RefCount==1.
-          // If this is a PTR_AND_OBJ entry, the OBJ is not part of the struct,
-          // so exclude it from this check.
-          int32_t parent_idx = getParentIndex(arg_types[i]);
-          uint64_t parent_rc = Device.getMapEntryRefCnt(args[parent_idx]);
-          assert(parent_rc > 0 && "parent struct not found");
-          if (parent_rc == 1) {
-            copy = true;
-          }
-        }
       }
 
       if (copy && !IsHostPtr) {
Index: openmp/libomptarget/src/device.h
===================================================================
--- openmp/libomptarget/src/device.h
+++ openmp/libomptarget/src/device.h
@@ -177,7 +177,6 @@
   // Return true if data can be copied to DstDevice directly
   bool isDataExchangable(const DeviceTy &DstDevice);
 
-  uint64_t getMapEntryRefCnt(void *HstPtrBegin);
   LookupResult lookupMapping(void *HstPtrBegin, int64_t Size);
   void *getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
                          map_var_info_t HstPtrName, bool &IsNew,
Index: openmp/libomptarget/src/device.cpp
===================================================================
--- openmp/libomptarget/src/device.cpp
+++ openmp/libomptarget/src/device.cpp
@@ -119,31 +119,6 @@
   return OFFLOAD_FAIL;
 }
 
-// Get ref count of map entry containing HstPtrBegin
-uint64_t DeviceTy::getMapEntryRefCnt(void *HstPtrBegin) {
-  uintptr_t hp = (uintptr_t)HstPtrBegin;
-  uint64_t RefCnt = 0;
-
-  DataMapMtx.lock();
-  if (!HostDataToTargetMap.empty()) {
-    auto upper = HostDataToTargetMap.upper_bound(hp);
-    if (upper != HostDataToTargetMap.begin()) {
-      upper--;
-      if (hp >= upper->HstPtrBegin && hp < upper->HstPtrEnd) {
-        DP("DeviceTy::getMapEntry: requested entry found\n");
-        RefCnt = upper->getRefCount();
-      }
-    }
-  }
-  DataMapMtx.unlock();
-
-  if (RefCnt == 0) {
-    DP("DeviceTy::getMapEntry: requested entry not found\n");
-  }
-
-  return RefCnt;
-}
-
 LookupResult DeviceTy::lookupMapping(void *HstPtrBegin, int64_t Size) {
   uintptr_t hp = (uintptr_t)HstPtrBegin;
   LookupResult lr;
@@ -214,9 +189,13 @@
   if (lr.Flags.IsContained ||
       ((lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) && IsImplicit)) {
     auto &HT = *lr.Entry;
-    IsNew = false;
+    assert(HT.getRefCount() > 0 && "expected existing RefCount > 0");
     if (UpdateRefCount)
+      // After this, RefCount > 1.
       HT.incRefCount();
+    else
+      // It might have been allocated with the parent, but it's still new.
+      IsNew = HT.getRefCount() == 1;
     uintptr_t tp = HT.TgtPtrBegin + ((uintptr_t)HstPtrBegin - HT.HstPtrBegin);
     INFO(OMP_INFOTYPE_MAPPING_EXISTS, DeviceID,
          "Mapping exists%s with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105121.355230.patch
Type: text/x-patch
Size: 3461 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20210629/347ddcf9/attachment.bin>