[Openmp-commits] [openmp] 369216a - [OpenMP][Offloading] Refined return value of `DeviceTy::getOrAllocTgtPtr`
Shilei Tian via Openmp-commits
openmp-commits at lists.llvm.org
Thu Jul 1 09:32:09 PDT 2021
Author: Shilei Tian
Date: 2021-07-01T12:32:03-04:00
New Revision: 369216ab3132623e98c8c974ded915803f080dcf
URL: https://github.com/llvm/llvm-project/commit/369216ab3132623e98c8c974ded915803f080dcf
DIFF: https://github.com/llvm/llvm-project/commit/369216ab3132623e98c8c974ded915803f080dcf.diff
LOG: [OpenMP][Offloading] Refined return value of `DeviceTy::getOrAllocTgtPtr`
`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).
Reviewed By: grokos
Differential Revision: https://reviews.llvm.org/D104382
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 36bf23d41bfd1..b1efb3dc6a7c8 100644
--- a/openmp/libomptarget/src/device.cpp
+++ b/openmp/libomptarget/src/device.cpp
@@ -191,50 +191,55 @@ LookupResult DeviceTy::lookupMapping(void *HstPtrBegin, int64_t Size) {
}
// 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,
+ bool UpdateRefCount, bool HasCloseModifier,
+ bool HasPresentModifier) {
+ 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)",
@@ -252,7 +257,7 @@ void *DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
"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 "
@@ -264,24 +269,22 @@ void *DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
} 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
- .emplace((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
- (uintptr_t)HstPtrBegin + Size, tp, HstPtrName)
- .first;
+ uintptr_t Ptr = (uintptr_t)allocData(Size, HstPtrBegin);
+ Entry = HostDataToTargetMap
+ .emplace((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
+ (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.
diff --git a/openmp/libomptarget/src/device.h b/openmp/libomptarget/src/device.h
index 69fc65d983d5c..3aa7f0b73c4f9 100644
--- a/openmp/libomptarget/src/device.h
+++ b/openmp/libomptarget/src/device.h
@@ -128,6 +128,23 @@ struct LookupResult {
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;
@@ -179,10 +196,12 @@ struct DeviceTy {
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,
diff --git a/openmp/libomptarget/src/omptarget.cpp b/openmp/libomptarget/src/omptarget.cpp
index dcc1f61dff327..e187e3d650d60 100644
--- a/openmp/libomptarget/src/omptarget.cpp
+++ b/openmp/libomptarget/src/omptarget.cpp
@@ -458,7 +458,7 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
// 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:
@@ -487,10 +487,11 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
// 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"
@@ -500,7 +501,7 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
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;
@@ -510,9 +511,11 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
(!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)) {
@@ -523,7 +526,7 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
}
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;
@@ -536,7 +539,7 @@ 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 (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)) {
More information about the Openmp-commits
mailing list