[OpenMP][Offloading] Refined return value of `DeviceTy::getOrAllocTgtPtr`

[tianshilei1992]

Summary

DeviceTy::getOrAllocTgtPtr just returns a target pointer. In addition,
two bool values (IsNew and IsHostPtr) are passed by reference to make the
change in the function available in callee.

In this patch, a struct, which contains the target pointer, two flags, and an
iterator to the map table entry corresponding to the queried host pointer, will
be returned. In addition to make the logic clearer regarding the two bool values,
this paves the way for the next patch to fix the data race in bug49334.cpp by
attaching an event to the map table entry (and that's why we need the iterator).

[grokos]

openmp/libomptarget/src/device.cpp

194–195

contains --> containing

283

I think emplace returns an iterator to the newly emplaced element. You can set Entry equal to the return value of this emplace, thus sparing a lookup a couple of lines below.

[tianshilei1992]

openmp/libomptarget/src/device.cpp

283

Thanks for the information. Yes, that will be better.

[protze.joachim]

Can you rebase this patch? I tried to apply to main and test, but there are couple of conflicts for device.cpp.

[tianshilei1992]

rebase

[tianshilei1992]

fix comments

[tianshilei1992]

remove unrelated change

[tianshilei1992]

rebase

[grokos]

I think this is good to go now. Thanks! Maybe you can wait for some of the initial reviewers to accept the patch as well or just go ahead and commit.

[tianshilei1992]

rebase

openmp/libomptarget/src/device.h

struct {
     unsigned ExtendsAfter : 1;
   } Flags;
 
   HostDataToTargetListTy::iterator Entry;
 
   LookupResult() : Flags({0, 0, 0}), Entry() {}
 };
 
+/// This struct will be returned by \p DeviceTy::getOrAllocTgtPtr which provides
+/// more data than just a target pointer.
+struct TargetPointerResultTy {
+  struct {
+    /// If the map table entry is just created
+    unsigned IsNewEntry : 1;
+    /// If the pointer is actually a host pointer (when unified memory enabled)
+    unsigned IsHostPointer : 1;
+  } Flags = {0, 0};
+
+  /// The iterator to the corresponding map table entry
+  HostDataToTargetListTy::iterator MapTableEntry{};
+
+  /// The corresponding target pointer
+  void *TargetPointer = nullptr;
+};
+
 /// Map for shadow pointers
 struct ShadowPtrValTy {
   void *HstPtrVal;
   void *TgtPtrAddr;
   void *TgtPtrVal;
 };
 typedef std::map<void *, ShadowPtrValTy> ShadowPtrListTy;
 
struct DeviceTy {
 
   ~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);
-  void *getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
-                         map_var_info_t HstPtrName, bool &IsNew,
-                         bool &IsHostPtr, bool IsImplicit, bool UpdateRefCount,
-                         bool HasCloseModifier, bool HasPresentModifier);
+  TargetPointerResultTy getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
+                                         int64_t Size,
+                                         map_var_info_t HstPtrName,
+                                         bool IsImplicit, bool UpdateRefCount,
+                                         bool HasCloseModifier,
+                                         bool HasPresentModifier);
   void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size);
   void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool &IsLast,
                        bool UpdateRefCount, bool &IsHostPtr,
                        bool MustContain = false, bool ForceDelete = false);
   int deallocTgtPtr(void *TgtPtrBegin, int64_t Size,
                     bool HasCloseModifier = false);
   int associatePtr(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size);
   int disassociatePtr(void *HstPtrBegin);

openmp/libomptarget/src/device.cpp

if (lr.Flags.ExtendsAfter) {
     DP("WARNING: Pointer is already mapped but section extends beyond mapped "
        "region\n");
   }
 
   return lr;
 }
 
 // Used by targetDataBegin
-// Return the target pointer begin (where the data will be moved).
+// Return a struct containing target pointer begin (where the data will be
+// moved).
 // Allocate memory if this is the first occurrence of this mapping.
 // Increment the reference counter.
-// If NULL is returned, then either data allocation failed or the user tried
-// to do an illegal mapping.
-void *DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
-                                 int64_t Size, map_var_info_t HstPtrName,
-                                 bool &IsNew, bool &IsHostPtr, bool IsImplicit,
-                                 bool UpdateRefCount, bool HasCloseModifier,
-                                 bool HasPresentModifier) {
-  void *rc = NULL;
-  IsHostPtr = false;
-  IsNew = false;
+// If the target pointer is NULL, then either data allocation failed or the user
+// tried to do an illegal mapping.
+// The returned struct also returns an iterator to the map table entry
+// corresponding to the host pointer (if exists), and two flags indicating
+// whether the entry is just created, and if the target pointer included is
+// actually a host pointer (when unified memory enabled).
+TargetPointerResultTy
+DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
+                           map_var_info_t HstPtrName, bool IsImplicit,
+  void *TargetPointer = NULL;
+  bool IsNew = false;
+  bool IsHostPtr = false;
   DataMapMtx.lock();
-  LookupResult lr = lookupMapping(HstPtrBegin, Size);
+  LookupResult LR = lookupMapping(HstPtrBegin, Size);
+  auto Entry = LR.Entry;
 
   // Check if the pointer is contained.
   // If a variable is mapped to the device manually by the user - which would
   // lead to the IsContained flag to be true - then we must ensure that the
   // device address is returned even under unified memory conditions.
-  if (lr.Flags.IsContained ||
-      ((lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) && IsImplicit)) {
-    auto &HT = *lr.Entry;
-    IsNew = false;
+  if (LR.Flags.IsContained ||
+      ((LR.Flags.ExtendsBefore || LR.Flags.ExtendsAfter) && IsImplicit)) {
+    auto &HT = *LR.Entry;
     if (UpdateRefCount)
       HT.incRefCount();
-    uintptr_t tp = HT.TgtPtrBegin + ((uintptr_t)HstPtrBegin - HT.HstPtrBegin);
+    uintptr_t Ptr = HT.TgtPtrBegin + ((uintptr_t)HstPtrBegin - HT.HstPtrBegin);
     INFO(OMP_INFOTYPE_MAPPING_EXISTS, DeviceID,
          "Mapping exists%s with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD
          ", "
          "Size=%" PRId64 ", RefCount=%s (%s), Name=%s\n",
-         (IsImplicit ? " (implicit)" : ""), DPxPTR(HstPtrBegin), DPxPTR(tp),
+         (IsImplicit ? " (implicit)" : ""), DPxPTR(HstPtrBegin), DPxPTR(Ptr),
          Size, HT.refCountToStr().c_str(),
          UpdateRefCount ? "incremented" : "update suppressed",
          (HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown");
-    rc = (void *)tp;
-  } else if ((lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) && !IsImplicit) {
+    TargetPointer = (void *)Ptr;
+  } else if ((LR.Flags.ExtendsBefore || LR.Flags.ExtendsAfter) && !IsImplicit) {
     // Explicit extension of mapped data - not allowed.
     MESSAGE("explicit extension not allowed: host address specified is " DPxMOD
             " (%" PRId64
             " bytes), but device allocation maps to host at " DPxMOD
             " (%" PRId64 " bytes)",
-            DPxPTR(HstPtrBegin), Size, DPxPTR(lr.Entry->HstPtrBegin),
-            lr.Entry->HstPtrEnd - lr.Entry->HstPtrBegin);
+            DPxPTR(HstPtrBegin), Size, DPxPTR(Entry->HstPtrBegin),
+            Entry->HstPtrEnd - Entry->HstPtrBegin);
     if (HasPresentModifier)
       MESSAGE("device mapping required by 'present' map type modifier does not "
               "exist for host address " DPxMOD " (%" PRId64 " bytes)",
               DPxPTR(HstPtrBegin), Size);
   } else if (PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
              !HasCloseModifier) {
     // If unified shared memory is active, implicitly mapped variables that are
     // not privatized use host address. Any explicitly mapped variables also use
     // host address where correctness is not impeded. In all other cases maps
     // are respected.
     // In addition to the mapping rules above, the close map modifier forces the
     // mapping of the variable to the device.
     if (Size) {
       DP("Return HstPtrBegin " DPxMOD " Size=%" PRId64 " for unified shared "
          "memory\n",
          DPxPTR((uintptr_t)HstPtrBegin), Size);
       IsHostPtr = true;
-      rc = HstPtrBegin;
+      TargetPointer = HstPtrBegin;
     }
   } else if (HasPresentModifier) {
     DP("Mapping required by 'present' map type modifier does not exist for "
        "HstPtrBegin=" DPxMOD ", Size=%" PRId64 "\n",
        DPxPTR(HstPtrBegin), Size);
     MESSAGE("device mapping required by 'present' map type modifier does not "
             "exist for host address " DPxMOD " (%" PRId64 " bytes)",
             DPxPTR(HstPtrBegin), Size);
   } else if (Size) {
     // If it is not contained and Size > 0, we should create a new entry for it.
     IsNew = true;
-    uintptr_t tp = (uintptr_t)allocData(Size, HstPtrBegin);
-    const HostDataToTargetTy &newEntry =
-        *HostDataToTargetMap
+    uintptr_t Ptr = (uintptr_t)allocData(Size, HstPtrBegin);
+    Entry = HostDataToTargetMap
                 .emplace((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
-                      (uintptr_t)HstPtrBegin + Size, tp, HstPtrName)
+                         (uintptr_t)HstPtrBegin + Size, Ptr, HstPtrName)
                 .first;
     INFO(OMP_INFOTYPE_MAPPING_CHANGED, DeviceID,
          "Creating new map entry with "
          "HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD ", Size=%ld, "
          "RefCount=%s, Name=%s\n",
-         DPxPTR(HstPtrBegin), DPxPTR(tp), Size,
-         newEntry.refCountToStr().c_str(),
+         DPxPTR(HstPtrBegin), DPxPTR(Ptr), Size, Entry->refCountToStr().c_str(),
          (HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown");
-    rc = (void *)tp;
+    TargetPointer = (void *)Ptr;
   }
 
   DataMapMtx.unlock();
-  return rc;
+  return {{IsNew, IsHostPtr}, Entry, TargetPointer};
 }
 
 // Used by targetDataBegin, targetDataEnd, targetDataUpdate and target.
 // Return the target pointer begin (where the data will be moved).
 // Decrement the reference counter if called from targetDataEnd.
 void *DeviceTy::getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool &IsLast,
                                bool UpdateRefCount, bool &IsHostPtr,
                                bool MustContain, bool ForceDelete) {

openmp/libomptarget/src/omptarget.cpp

if (getParentIndex(arg_types[i]) < 0 && next_i < arg_num &&
            padding, DPxPTR(HstPtrBegin));
         HstPtrBegin = (char *)HstPtrBegin - padding;
         data_size += padding;
       }
     }
 
     // Address of pointer on the host and device, respectively.
     void *Pointer_HstPtrBegin, *PointerTgtPtrBegin;
-    bool IsNew, Pointer_IsNew;
+    TargetPointerResultTy Pointer_TPR;
     bool IsHostPtr = false;
     bool IsImplicit = arg_types[i] & OMP_TGT_MAPTYPE_IMPLICIT;
     // Force the creation of a device side copy of the data when:
     // a close map modifier was associated with a map that contained a to.
     bool HasCloseModifier = arg_types[i] & OMP_TGT_MAPTYPE_CLOSE;
     bool HasPresentModifier = arg_types[i] & OMP_TGT_MAPTYPE_PRESENT;
     // UpdateRef is based on MEMBER_OF instead of TARGET_PARAM because if we
     // have reached this point via __tgt_target_data_begin and not __tgt_target
if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ) {
       //
       // The map entry for s comes first, and the PTR_AND_OBJ entry comes
       // afterward, so the pointer is already allocated by the time the
       // PTR_AND_OBJ entry is handled below, and PointerTgtPtrBegin is thus
       // non-null.  However, "declare target link" can produce a PTR_AND_OBJ
       // entry for a global that might not already be allocated by the time the
       // PTR_AND_OBJ entry is handled below, and so the allocation might fail
       // when HasPresentModifier.
-      PointerTgtPtrBegin = Device.getOrAllocTgtPtr(
-          HstPtrBase, HstPtrBase, sizeof(void *), nullptr, Pointer_IsNew,
-          IsHostPtr, IsImplicit, UpdateRef, HasCloseModifier,
-          HasPresentModifier);
+      Pointer_TPR = Device.getOrAllocTgtPtr(
+          HstPtrBase, HstPtrBase, sizeof(void *), nullptr, IsImplicit,
+          UpdateRef, HasCloseModifier, HasPresentModifier);
+      PointerTgtPtrBegin = Pointer_TPR.TargetPointer;
+      IsHostPtr = Pointer_TPR.Flags.IsHostPointer;
       if (!PointerTgtPtrBegin) {
         REPORT("Call to getOrAllocTgtPtr returned null pointer (%s).\n",
                HasPresentModifier ? "'present' map type modifier"
                                   : "device failure or illegal mapping");
         return OFFLOAD_FAIL;
       }
       DP("There are %zu bytes allocated at target address " DPxMOD " - is%s new"
          "\n",
          sizeof(void *), DPxPTR(PointerTgtPtrBegin),
-         (Pointer_IsNew ? "" : " not"));
+         (Pointer_TPR.Flags.IsNewEntry ? "" : " not"));
       Pointer_HstPtrBegin = HstPtrBase;
       // modify current entry.
       HstPtrBase = *(void **)HstPtrBase;
       // No need to update pointee ref count for the first element of the
       // subelement that comes from mapper.
       UpdateRef =
           (!FromMapper || i != 0); // subsequently update ref count of pointee
     }
 
-    void *TgtPtrBegin = Device.getOrAllocTgtPtr(
-        HstPtrBegin, HstPtrBase, data_size, HstPtrName, IsNew, IsHostPtr,
-        IsImplicit, UpdateRef, HasCloseModifier, HasPresentModifier);
+    auto TPR = Device.getOrAllocTgtPtr(HstPtrBegin, HstPtrBase, data_size,
+                                       HstPtrName, IsImplicit, UpdateRef,
+                                       HasCloseModifier, HasPresentModifier);
+    void *TgtPtrBegin = TPR.TargetPointer;
+    IsHostPtr = TPR.Flags.IsHostPointer;
     // If data_size==0, then the argument could be a zero-length pointer to
     // NULL, so getOrAlloc() returning NULL is not an error.
     if (!TgtPtrBegin && (data_size || HasPresentModifier)) {
       REPORT("Call to getOrAllocTgtPtr returned null pointer (%s).\n",
              HasPresentModifier ? "'present' map type modifier"
                                 : "device failure or illegal mapping");
       return OFFLOAD_FAIL;
     }
     DP("There are %" PRId64 " bytes allocated at target address " DPxMOD
        " - is%s new\n",
-       data_size, DPxPTR(TgtPtrBegin), (IsNew ? "" : " not"));
+       data_size, DPxPTR(TgtPtrBegin), (TPR.Flags.IsNewEntry ? "" : " not"));
 
     if (arg_types[i] & OMP_TGT_MAPTYPE_RETURN_PARAM) {
       uintptr_t Delta = (uintptr_t)HstPtrBegin - (uintptr_t)HstPtrBase;
       void *TgtPtrBase = (void *)((uintptr_t)TgtPtrBegin - Delta);
       DP("Returning device pointer " DPxMOD "\n", DPxPTR(TgtPtrBase));
       args_base[i] = TgtPtrBase;
     }
 
     if (arg_types[i] & OMP_TGT_MAPTYPE_TO) {
       bool copy = false;
       if (!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
           HasCloseModifier) {
-        if (IsNew || (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]);