[Openmp-commits] [openmp] d99f65d - [OpenMP] Avoid checking parent reference count in targetDataBegin

[Joel E. Denny via Openmp-commits]

Author: Joel E. Denny
Date: 2021-07-10T12:15:04-04:00
New Revision: d99f65de2ab1765c588688876641f5018bfe3b53

URL: https://github.com/llvm/llvm-project/commit/d99f65de2ab1765c588688876641f5018bfe3b53
DIFF: https://github.com/llvm/llvm-project/commit/d99f65de2ab1765c588688876641f5018bfe3b53.diff

LOG: [OpenMP] Avoid checking parent reference count in targetDataBegin

This patch is an attempt to do for `targetDataBegin` what 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 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.

Reviewed By: jdoerfert, jhuber6, protze.joachim, tianshilei1992, grokos, RaviNarayanaswamy

Differential Revision: https://reviews.llvm.org/D105121

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/openmp/libomptarget/src/device.cpp b/openmp/libomptarget/src/device.cpp
index b1efb3dc6a7c..33d16fb5d548 100644
--- a/openmp/libomptarget/src/device.cpp
+++ b/openmp/libomptarget/src/device.cpp
@@ -119,31 +119,6 @@ int DeviceTy::disassociatePtr(void *HstPtrBegin) {
   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;
@@ -220,8 +195,13 @@ DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
   if (LR.Flags.IsContained ||
       ((LR.Flags.ExtendsBefore || LR.Flags.ExtendsAfter) && IsImplicit)) {
     auto &HT = *LR.Entry;
+    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 Ptr = HT.TgtPtrBegin + ((uintptr_t)HstPtrBegin - HT.HstPtrBegin);
     INFO(OMP_INFOTYPE_MAPPING_EXISTS, DeviceID,
          "Mapping exists%s with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD

diff  --git a/openmp/libomptarget/src/device.h b/openmp/libomptarget/src/device.h
index 3aa7f0b73c4f..dee4325822ba 100644
--- a/openmp/libomptarget/src/device.h
+++ b/openmp/libomptarget/src/device.h
@@ -194,7 +194,6 @@ struct DeviceTy {
   // 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);
   TargetPointerResultTy getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
                                          int64_t Size,

diff  --git a/openmp/libomptarget/src/omptarget.cpp b/openmp/libomptarget/src/omptarget.cpp
index 19a3c4ddad69..80cb9381ffbb 100644
--- a/openmp/libomptarget/src/omptarget.cpp
+++ b/openmp/libomptarget/src/omptarget.cpp
@@ -539,20 +539,8 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
       bool copy = false;
       if (!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
           HasCloseModifier) {
-        if (TPR.Flags.IsNewEntry || (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)) {
+        if (TPR.Flags.IsNewEntry || (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) {