replica
/
hanchenye-llvm-project
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

[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
This commit is contained in:
Shilei Tian 2021-07-01 12:31:45 -04:00
parent 266a7414d8
commit 369216ab31
3 changed files with 73 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
openmp/libomptarget/src/device.cpp
View File

@ -191,50 +191,55 @@ LookupResult DeviceTy::lookupMapping(void *HstPtrBegin, int64_t Size) {
} }
// Used by targetDataBegin // 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. // Allocate memory if this is the first occurrence of this mapping.
// Increment the reference counter. // Increment the reference counter.
// If NULL is returned, then either data allocation failed or the user tried // If the target pointer is NULL, then either data allocation failed or the user
// to do an illegal mapping. // tried to do an illegal mapping.
void *DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, // The returned struct also returns an iterator to the map table entry
int64_t Size, map_var_info_t HstPtrName, // corresponding to the host pointer (if exists), and two flags indicating
bool &IsNew, bool &IsHostPtr, bool IsImplicit, // whether the entry is just created, and if the target pointer included is
bool UpdateRefCount, bool HasCloseModifier, // actually a host pointer (when unified memory enabled).
bool HasPresentModifier) { TargetPointerResultTy
void *rc = NULL; DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
IsHostPtr = false; map_var_info_t HstPtrName, bool IsImplicit,
IsNew = false; bool UpdateRefCount, bool HasCloseModifier,
bool HasPresentModifier) {
void *TargetPointer = NULL;
bool IsNew = false;
bool IsHostPtr = false;
DataMapMtx.lock(); DataMapMtx.lock();
LookupResult lr = lookupMapping(HstPtrBegin, Size); LookupResult LR = lookupMapping(HstPtrBegin, Size);
auto Entry = LR.Entry;
// Check if the pointer is contained. // Check if the pointer is contained.
// If a variable is mapped to the device manually by the user - which would // 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 // lead to the IsContained flag to be true - then we must ensure that the
// device address is returned even under unified memory conditions. // device address is returned even under unified memory conditions.
if (lr.Flags.IsContained || if (LR.Flags.IsContained ||
((lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) && IsImplicit)) { ((LR.Flags.ExtendsBefore || LR.Flags.ExtendsAfter) && IsImplicit)) {
auto &HT = *lr.Entry; auto &HT = *LR.Entry;
IsNew = false;
if (UpdateRefCount) if (UpdateRefCount)
HT.incRefCount(); 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, INFO(OMP_INFOTYPE_MAPPING_EXISTS, DeviceID,
"Mapping exists%s with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD "Mapping exists%s with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD
", " ", "
"Size=%" PRId64 ", RefCount=%s (%s), Name=%s\n", "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(), Size, HT.refCountToStr().c_str(),
UpdateRefCount ? "incremented" : "update suppressed", UpdateRefCount ? "incremented" : "update suppressed",
(HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown"); (HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown");
rc = (void *)tp; TargetPointer = (void *)Ptr;
} else if ((lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) && !IsImplicit) { } else if ((LR.Flags.ExtendsBefore || LR.Flags.ExtendsAfter) && !IsImplicit) {
// Explicit extension of mapped data - not allowed. // Explicit extension of mapped data - not allowed.
MESSAGE("explicit extension not allowed: host address specified is " DPxMOD MESSAGE("explicit extension not allowed: host address specified is " DPxMOD
" (%" PRId64 " (%" PRId64
" bytes), but device allocation maps to host at " DPxMOD " bytes), but device allocation maps to host at " DPxMOD
" (%" PRId64 " bytes)", " (%" PRId64 " bytes)",
DPxPTR(HstPtrBegin), Size, DPxPTR(lr.Entry->HstPtrBegin), DPxPTR(HstPtrBegin), Size, DPxPTR(Entry->HstPtrBegin),
lr.Entry->HstPtrEnd - lr.Entry->HstPtrBegin); Entry->HstPtrEnd - Entry->HstPtrBegin);
if (HasPresentModifier) if (HasPresentModifier)
MESSAGE("device mapping required by 'present' map type modifier does not " MESSAGE("device mapping required by 'present' map type modifier does not "
"exist for host address " DPxMOD " (%" PRId64 " bytes)", "exist for host address " DPxMOD " (%" PRId64 " bytes)",
@ -252,7 +257,7 @@ void *DeviceTy::getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
"memory\n", "memory\n",
DPxPTR((uintptr_t)HstPtrBegin), Size); DPxPTR((uintptr_t)HstPtrBegin), Size);
IsHostPtr = true; IsHostPtr = true;
rc = HstPtrBegin; TargetPointer = HstPtrBegin;
} }
} else if (HasPresentModifier) { } else if (HasPresentModifier) {
DP("Mapping required by 'present' map type modifier does not exist for " 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) { } else if (Size) {
// If it is not contained and Size > 0, we should create a new entry for it. // If it is not contained and Size > 0, we should create a new entry for it.
IsNew = true; IsNew = true;
uintptr_t tp = (uintptr_t)allocData(Size, HstPtrBegin); uintptr_t Ptr = (uintptr_t)allocData(Size, HstPtrBegin);
const HostDataToTargetTy &newEntry = Entry = HostDataToTargetMap
*HostDataToTargetMap .emplace((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
.emplace((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin, (uintptr_t)HstPtrBegin + Size, Ptr, HstPtrName)
(uintptr_t)HstPtrBegin + Size, tp, HstPtrName) .first;
.first;
INFO(OMP_INFOTYPE_MAPPING_CHANGED, DeviceID, INFO(OMP_INFOTYPE_MAPPING_CHANGED, DeviceID,
"Creating new map entry with " "Creating new map entry with "
"HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD ", Size=%ld, " "HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD ", Size=%ld, "
"RefCount=%s, Name=%s\n", "RefCount=%s, Name=%s\n",
DPxPTR(HstPtrBegin), DPxPTR(tp), Size, DPxPTR(HstPtrBegin), DPxPTR(Ptr), Size, Entry->refCountToStr().c_str(),
newEntry.refCountToStr().c_str(),
(HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown"); (HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown");
rc = (void *)tp; TargetPointer = (void *)Ptr;
} }
DataMapMtx.unlock(); DataMapMtx.unlock();
return rc; return {{IsNew, IsHostPtr}, Entry, TargetPointer};
} }
// Used by targetDataBegin, targetDataEnd, targetDataUpdate and target. // Used by targetDataBegin, targetDataEnd, targetDataUpdate and target.

27
openmp/libomptarget/src/device.h
View File

@ -128,6 +128,23 @@ struct LookupResult {
LookupResult() : Flags({0, 0, 0}), 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 /// Map for shadow pointers
struct ShadowPtrValTy { struct ShadowPtrValTy {
void *HstPtrVal; void *HstPtrVal;
@ -179,10 +196,12 @@ struct DeviceTy {
uint64_t getMapEntryRefCnt(void *HstPtrBegin); uint64_t getMapEntryRefCnt(void *HstPtrBegin);
LookupResult lookupMapping(void *HstPtrBegin, int64_t Size); LookupResult lookupMapping(void *HstPtrBegin, int64_t Size);
void *getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase, int64_t Size, TargetPointerResultTy getOrAllocTgtPtr(void *HstPtrBegin, void *HstPtrBase,
map_var_info_t HstPtrName, bool &IsNew, int64_t Size,
bool &IsHostPtr, bool IsImplicit, bool UpdateRefCount, map_var_info_t HstPtrName,
bool HasCloseModifier, bool HasPresentModifier); bool IsImplicit, bool UpdateRefCount,
bool HasCloseModifier,
bool HasPresentModifier);
void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size); void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size);
void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool &IsLast, void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool &IsLast,
bool UpdateRefCount, bool &IsHostPtr, bool UpdateRefCount, bool &IsHostPtr,

25
openmp/libomptarget/src/omptarget.cpp
View File

@ -458,7 +458,7 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
// Address of pointer on the host and device, respectively. // Address of pointer on the host and device, respectively.
void *Pointer_HstPtrBegin, *PointerTgtPtrBegin; void *Pointer_HstPtrBegin, *PointerTgtPtrBegin;
bool IsNew, Pointer_IsNew; TargetPointerResultTy Pointer_TPR;
bool IsHostPtr = false; bool IsHostPtr = false;
bool IsImplicit = arg_types[i] & OMP_TGT_MAPTYPE_IMPLICIT; bool IsImplicit = arg_types[i] & OMP_TGT_MAPTYPE_IMPLICIT;
// Force the creation of a device side copy of the data when: // 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 // 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 // PTR_AND_OBJ entry is handled below, and so the allocation might fail
// when HasPresentModifier. // when HasPresentModifier.
PointerTgtPtrBegin = Device.getOrAllocTgtPtr( Pointer_TPR = Device.getOrAllocTgtPtr(
HstPtrBase, HstPtrBase, sizeof(void *), nullptr, Pointer_IsNew, HstPtrBase, HstPtrBase, sizeof(void *), nullptr, IsImplicit,
IsHostPtr, IsImplicit, UpdateRef, HasCloseModifier, UpdateRef, HasCloseModifier, HasPresentModifier);
HasPresentModifier); PointerTgtPtrBegin = Pointer_TPR.TargetPointer;
IsHostPtr = Pointer_TPR.Flags.IsHostPointer;
if (!PointerTgtPtrBegin) { if (!PointerTgtPtrBegin) {
REPORT("Call to getOrAllocTgtPtr returned null pointer (%s).\n", REPORT("Call to getOrAllocTgtPtr returned null pointer (%s).\n",
HasPresentModifier ? "'present' map type modifier" 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" DP("There are %zu bytes allocated at target address " DPxMOD " - is%s new"
"\n", "\n",
sizeof(void *), DPxPTR(PointerTgtPtrBegin), sizeof(void *), DPxPTR(PointerTgtPtrBegin),
(Pointer_IsNew ? "" : " not")); (Pointer_TPR.Flags.IsNewEntry ? "" : " not"));
Pointer_HstPtrBegin = HstPtrBase; Pointer_HstPtrBegin = HstPtrBase;
// modify current entry. // modify current entry.
HstPtrBase = *(void **)HstPtrBase; 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 (!FromMapper || i != 0); // subsequently update ref count of pointee
} }
void *TgtPtrBegin = Device.getOrAllocTgtPtr( auto TPR = Device.getOrAllocTgtPtr(HstPtrBegin, HstPtrBase, data_size,
HstPtrBegin, HstPtrBase, data_size, HstPtrName, IsNew, IsHostPtr, HstPtrName, IsImplicit, UpdateRef,
IsImplicit, UpdateRef, HasCloseModifier, HasPresentModifier); HasCloseModifier, HasPresentModifier);
void *TgtPtrBegin = TPR.TargetPointer;
IsHostPtr = TPR.Flags.IsHostPointer;
// If data_size==0, then the argument could be a zero-length pointer to // If data_size==0, then the argument could be a zero-length pointer to
// NULL, so getOrAlloc() returning NULL is not an error. // NULL, so getOrAlloc() returning NULL is not an error.
if (!TgtPtrBegin && (data_size || HasPresentModifier)) { 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 DP("There are %" PRId64 " bytes allocated at target address " DPxMOD
" - is%s new\n", " - 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) { if (arg_types[i] & OMP_TGT_MAPTYPE_RETURN_PARAM) {
uintptr_t Delta = (uintptr_t)HstPtrBegin - (uintptr_t)HstPtrBase; 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; bool copy = false;
if (!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) || if (!(PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) ||
HasCloseModifier) { HasCloseModifier) {
if (IsNew || (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)) { if (TPR.Flags.IsNewEntry || (arg_types[i] & OMP_TGT_MAPTYPE_ALWAYS)) {
copy = true; copy = true;
} else if ((arg_types[i] & OMP_TGT_MAPTYPE_MEMBER_OF) && } else if ((arg_types[i] & OMP_TGT_MAPTYPE_MEMBER_OF) &&
!(arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ)) { !(arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ)) {