[Openmp-commits] [openmp] 1e447d0 - [OpenMP] Introduce an environment variable to disable atomic map clauses
Johannes Doerfert via Openmp-commits
openmp-commits at lists.llvm.org
Wed Jan 19 20:14:52 PST 2022
Author: Johannes Doerfert
Date: 2022-01-19T22:14:41-06:00
New Revision: 1e447d03e2f634b7772b8f2c52a8a2cbafa51f19
URL: https://github.com/llvm/llvm-project/commit/1e447d03e2f634b7772b8f2c52a8a2cbafa51f19
DIFF: https://github.com/llvm/llvm-project/commit/1e447d03e2f634b7772b8f2c52a8a2cbafa51f19.diff
LOG: [OpenMP] Introduce an environment variable to disable atomic map clauses
Atomic handling of map clauses was introduced to comply with the OpenMP
standard (see D104418). However, many apps won't need this feature which
can be costly in certain situations. To allow for applications to
opt-out we now introduce the `LIBOMPTARGET_MAP_FORCE_ATOMIC` environment
flag that voids the atomicity guarantee of the standard for map clauses
again, shifting the burden to the user.
This patch also de-duplicates the code that introduces the events used
to enforce atomicity as a cleanup.
Differential Revision: https://reviews.llvm.org/D117627
Added:
Modified:
openmp/docs/design/Runtimes.rst
openmp/libomptarget/include/device.h
openmp/libomptarget/src/device.cpp
openmp/libomptarget/src/omptarget.cpp
openmp/libomptarget/src/rtl.cpp
Removed:
################################################################################
diff --git a/openmp/docs/design/Runtimes.rst b/openmp/docs/design/Runtimes.rst
index 573de83a8fa25..d8140ea8cfeb1 100644
--- a/openmp/docs/design/Runtimes.rst
+++ b/openmp/docs/design/Runtimes.rst
@@ -683,6 +683,7 @@ variables is defined below.
* ``LIBOMPTARGET_HEAP_SIZE=<Num>``
* ``LIBOMPTARGET_STACK_SIZE=<Num>``
* ``LIBOMPTARGET_SHARED_MEMORY_SIZE=<Num>``
+ * ``LIBOMPTARGET_MAP_FORCE_ATOMIC=[TRUE/FALSE] (default TRUE)``
LIBOMPTARGET_DEBUG
""""""""""""""""""
@@ -1003,6 +1004,23 @@ runtime call.
Offloading
+
+LIBOMPTARGET_MAP_FORCE_ATOMIC
+"""""""""""""""""""""""""""""
+
+The OpenMP standard guarantees that map clauses are atomic. However, the this
+can have a drastic performance impact. Users that do not require atomic map
+clauses can disable them to potentially recover lost performance. As a
+consequence, users have to guarantee themselves that no two map clauses will
+concurrently map the same memory. If the memory is already mapped and the
+map clauses will only modify the reference counter from a non-zero count to
+another non-zero count, concurrent map clauses are supported regardless of
+this option. To disable forced atomic map clauses use "false"/"FALSE" as the
+value of the ``LIBOMPTARGET_MAP_FORCE_ATOMIC`` environment variable.
+The default behavior of LLVM 14 is to force atomic maps clauses, prior versions
+of LLVM did not.
+
+
LLVM/OpenMP Target Host Runtime Plugins (``libomptarget.rtl.XXXX``)
-------------------------------------------------------------------
diff --git a/openmp/libomptarget/include/device.h b/openmp/libomptarget/include/device.h
index dbaa1bd0b460a..313b25b2c1d8f 100644
--- a/openmp/libomptarget/include/device.h
+++ b/openmp/libomptarget/include/device.h
@@ -126,6 +126,10 @@ struct HostDataToTargetTy {
/// Get the event bound to this data map.
void *getEvent() const { return States->Event; }
+ /// Add a new event, if necessary.
+ /// Returns OFFLOAD_FAIL if something went wrong, OFFLOAD_SUCCESS otherwise.
+ int addEventIfNecessary(DeviceTy &Device, AsyncInfoTy &AsyncInfo) const;
+
/// Set the event bound to this data map.
void setEvent(void *Event) const { States->Event = Event; }
@@ -192,6 +196,18 @@ struct HostDataToTargetTy {
return ThisRefCount == 1;
}
+ /// Helper to make sure the entry is locked in a scope.
+ /// TODO: We should generalize this and use it for all our objects that use
+ /// lock/unlock methods.
+ struct LockGuard {
+ const HostDataToTargetTy &Entry;
+
+ public:
+ LockGuard(const HostDataToTargetTy &Entry) : Entry(Entry) { Entry.lock(); }
+ ~LockGuard() { Entry.unlock(); }
+ };
+
+private:
void lock() const { States->UpdateMtx.lock(); }
void unlock() const { States->UpdateMtx.unlock(); }
@@ -396,6 +412,9 @@ extern bool device_is_ready(int device_num);
/// Struct for the data required to handle plugins
struct PluginManager {
+ PluginManager(bool UseEventsForAtomicTransfers)
+ : UseEventsForAtomicTransfers(UseEventsForAtomicTransfers) {}
+
/// RTLs identified on the host
RTLsTy RTLs;
@@ -416,6 +435,10 @@ struct PluginManager {
// Store target policy (disabled, mandatory, default)
kmp_target_offload_kind_t TargetOffloadPolicy = tgt_default;
std::mutex TargetOffloadMtx; ///< For TargetOffloadPolicy
+
+ /// Flag to indicate if we use events to ensure the atomicity of
+ /// map clauses or not. Can be modified with an environment variable.
+ const bool UseEventsForAtomicTransfers;
};
extern PluginManager *PM;
diff --git a/openmp/libomptarget/src/device.cpp b/openmp/libomptarget/src/device.cpp
index 738284e82f20b..a3f373f8dd53e 100644
--- a/openmp/libomptarget/src/device.cpp
+++ b/openmp/libomptarget/src/device.cpp
@@ -19,6 +19,33 @@
#include <cstdio>
#include <string>
+int HostDataToTargetTy::addEventIfNecessary(
+ DeviceTy &Device, AsyncInfoTy &AsyncInfo) const {
+ // First, check if the user disabled atomic map transfer/malloc/dealloc.
+ if (!PM->UseEventsForAtomicTransfers)
+ return OFFLOAD_SUCCESS;
+
+ void *Event = getEvent();
+ bool NeedNewEvent = Event == nullptr;
+ if (NeedNewEvent && Device.createEvent(&Event) != OFFLOAD_SUCCESS) {
+ REPORT("Failed to create event\n");
+ return OFFLOAD_FAIL;
+ }
+
+ // We cannot assume the event should not be nullptr because we don't
+ // know if the target support event. But if a target doesn't,
+ // recordEvent should always return success.
+ if (Device.recordEvent(Event, AsyncInfo) != OFFLOAD_SUCCESS) {
+ REPORT("Failed to set dependence on event " DPxMOD "\n", DPxPTR(Event));
+ return OFFLOAD_FAIL;
+ }
+
+ if (NeedNewEvent)
+ setEvent(Event);
+
+ return OFFLOAD_SUCCESS;
+}
+
DeviceTy::DeviceTy(RTLInfoTy *RTL)
: DeviceID(-1), RTL(RTL), RTLDeviceID(-1), IsInit(false), InitFlag(),
HasPendingGlobals(false), HostDataToTargetMap(), PendingCtorsDtors(),
@@ -259,7 +286,7 @@ DeviceTy::getTargetPointer(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
if (TargetPointer && !IsHostPtr && HasFlagTo && (IsNew || HasFlagAlways)) {
// Lock the entry before releasing the mapping table lock such that another
// thread that could issue data movement will get the right result.
- Entry->lock();
+ HostDataToTargetTy::LockGuard LG(*Entry);
// Release the mapping table lock right after the entry is locked.
DataMapMtx.unlock();
@@ -268,38 +295,16 @@ DeviceTy::getTargetPointer(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
int Ret = submitData(TargetPointer, HstPtrBegin, Size, AsyncInfo);
if (Ret != OFFLOAD_SUCCESS) {
- Entry->unlock();
REPORT("Copying data to device failed.\n");
// We will also return nullptr if the data movement fails because that
// pointer points to a corrupted memory region so it doesn't make any
// sense to continue to use it.
TargetPointer = nullptr;
- }
-
- void *Event = Entry->getEvent();
- bool NeedNewEvent = Event == nullptr;
- if (NeedNewEvent && createEvent(&Event) != OFFLOAD_SUCCESS) {
- Entry->unlock();
- REPORT("Failed to create event\n");
- return {{false /* IsNewEntry */, false /* IsHostPointer */},
- {} /* MapTableEntry */,
- nullptr /* TargetPointer */};
- }
- // We cannot assume the event should not be nullptr because we don't
- // know if the target support event. But if a target doesn't,
- // recordEvent should always return success.
- Ret = recordEvent(Event, AsyncInfo);
- if (Ret != OFFLOAD_SUCCESS) {
- Entry->unlock();
- REPORT("Failed to set dependence on event " DPxMOD "\n", DPxPTR(Event));
+ } else if (Entry->addEventIfNecessary(*this, AsyncInfo) !=
+ OFFLOAD_SUCCESS)
return {{false /* IsNewEntry */, false /* IsHostPointer */},
{} /* MapTableEntry */,
nullptr /* TargetPointer */};
- }
- if (NeedNewEvent)
- Entry->setEvent(Event);
- // We're done with the entry. Release the entry.
- Entry->unlock();
} else {
// Release the mapping table lock directly.
DataMapMtx.unlock();
@@ -308,11 +313,10 @@ DeviceTy::getTargetPointer(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
// Note: Entry might be nullptr because of zero length array section.
if (Entry != HostDataToTargetListTy::iterator() && !IsHostPtr &&
!HasPresentModifier) {
- Entry->lock();
+ HostDataToTargetTy::LockGuard LG(*Entry);
void *Event = Entry->getEvent();
if (Event) {
int Ret = waitEvent(Event, AsyncInfo);
- Entry->unlock();
if (Ret != OFFLOAD_SUCCESS) {
// If it fails to wait for the event, we need to return nullptr in
// case of any data race.
@@ -321,8 +325,6 @@ DeviceTy::getTargetPointer(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
{} /* MapTableEntry */,
nullptr /* TargetPointer */};
}
- } else {
- Entry->unlock();
}
}
}
diff --git a/openmp/libomptarget/src/omptarget.cpp b/openmp/libomptarget/src/omptarget.cpp
index dd3f97e12f724..144ac3d895012 100644
--- a/openmp/libomptarget/src/omptarget.cpp
+++ b/openmp/libomptarget/src/omptarget.cpp
@@ -572,7 +572,7 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
}
if (UpdateDevPtr) {
- Pointer_TPR.MapTableEntry->lock();
+ HostDataToTargetTy::LockGuard LG(*Pointer_TPR.MapTableEntry);
Device.ShadowMtx.unlock();
DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",
@@ -584,30 +584,12 @@ int targetDataBegin(ident_t *loc, DeviceTy &Device, int32_t arg_num,
int Ret = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
sizeof(void *), AsyncInfo);
if (Ret != OFFLOAD_SUCCESS) {
- Pointer_TPR.MapTableEntry->unlock();
REPORT("Copying data to device failed.\n");
return OFFLOAD_FAIL;
}
- void *Event = Pointer_TPR.MapTableEntry->getEvent();
- bool NeedNewEvent = Event == nullptr;
- if (NeedNewEvent && Device.createEvent(&Event) != OFFLOAD_SUCCESS) {
- Pointer_TPR.MapTableEntry->unlock();
- REPORT("Failed to create event.\n");
+ if (Pointer_TPR.MapTableEntry->addEventIfNecessary(Device, AsyncInfo) !=
+ OFFLOAD_SUCCESS)
return OFFLOAD_FAIL;
- }
- // We cannot assume the event should not be nullptr because we don't
- // know if the target support event. But if a target doesn't,
- // recordEvent should always return success.
- Ret = Device.recordEvent(Event, AsyncInfo);
- if (Ret != OFFLOAD_SUCCESS) {
- Pointer_TPR.MapTableEntry->unlock();
- REPORT("Failed to set dependence on event " DPxMOD "\n",
- DPxPTR(Event));
- return OFFLOAD_FAIL;
- }
- if (NeedNewEvent)
- Pointer_TPR.MapTableEntry->setEvent(Event);
- Pointer_TPR.MapTableEntry->unlock();
} else
Device.ShadowMtx.unlock();
}
diff --git a/openmp/libomptarget/src/rtl.cpp b/openmp/libomptarget/src/rtl.cpp
index e0d0e8122668f..2201575371681 100644
--- a/openmp/libomptarget/src/rtl.cpp
+++ b/openmp/libomptarget/src/rtl.cpp
@@ -40,7 +40,20 @@ static char *ProfileTraceFile = nullptr;
__attribute__((constructor(101))) void init() {
DP("Init target library!\n");
- PM = new PluginManager();
+
+ bool UseEventsForAtomicTransfers = true;
+ if (const char *ForceAtomicMap = getenv("LIBOMPTARGET_MAP_FORCE_ATOMIC")) {
+ std::string ForceAtomicMapStr(ForceAtomicMap);
+ if (ForceAtomicMapStr == "false" || ForceAtomicMapStr == "FALSE")
+ UseEventsForAtomicTransfers = false;
+ else if (ForceAtomicMapStr != "true" && ForceAtomicMapStr != "TRUE")
+ fprintf(stderr,
+ "Warning: 'LIBOMPTARGET_MAP_FORCE_ATOMIC' accepts only "
+ "'true'/'TRUE' or 'false'/'FALSE' as options, '%s' ignored\n",
+ ForceAtomicMap);
+ }
+
+ PM = new PluginManager(UseEventsForAtomicTransfers);
#ifdef OMPTARGET_PROFILE_ENABLED
ProfileTraceFile = getenv("LIBOMPTARGET_PROFILE");
More information about the Openmp-commits
mailing list