[OpenMP] Introduce an environment variable to disable atomic map clauses

[jdoerfert]

Summary

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.

[tianshilei1992]

openmp/libomptarget/src/device.cpp

22

This is arguably not a good practice to handle locks. We have Entry->lock() in one place and then nothing. For sure the function name addEventIfNecessaryAndUnlock contains "unlock", but it's still too vague because locks are usually used by pair or guarded by scope. Although I agree the code looks more neat, I don't think it's worth to do it in exchange of ambiguity.

[jdoerfert]

Address comments, use lock guard

[jdoerfert]

openmp/libomptarget/src/device.cpp

22

done

[tianshilei1992]

LGTM

openmp/docs/design/Runtimes.rst

1015

Probably better to mention no matter implicitly or explicitly.

openmp/libomptarget/include/device.h

215

That's a beautiful solution!

[jdoerfert]

openmp/docs/design/Runtimes.rst

1015

I don't get that. What to mention and how?

[ye-luo]

openmp/docs/design/Runtimes.rst

686

OpenMP spec use true/false, TRUE/FALSE. So better to keep the same convention. Values other than these four values should error out.

openmp/libomptarget/src/device.cpp

26

unlock before return.

[ye-luo]

Please update the allowed string values and also resolve conflict before landing.

[tianshilei1992]

openmp/docs/design/Runtimes.rst

686

libomp supports all of them. Better to align with them.

1015

"As a consequence, users have to guarantee themselves that no two data mappings of same memory, explicitly through map clause or implicitly applied by compiler, will concurrently happen."

[jdoerfert]

Address comments

[jdoerfert]

openmp/libomptarget/src/device.cpp

26

guards fix this nicely :)

[jdoerfert]

Typo

openmp/docs/design/Runtimes.rst

variables is defined below.
 
     * ``LIBOMPTARGET_DEBUG=<Num>``
     * ``LIBOMPTARGET_PROFILE=<Filename>``
     * ``LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD=<Num>``
     * ``LIBOMPTARGET_INFO=<Num>``
     * ``LIBOMPTARGET_HEAP_SIZE=<Num>``
     * ``LIBOMPTARGET_STACK_SIZE=<Num>``
     * ``LIBOMPTARGET_SHARED_MEMORY_SIZE=<Num>``
+    * ``LIBOMPTARGET_MAP_FORCE_ATOMIC=[TRUE/FALSE] (default TRUE)``
 
 LIBOMPTARGET_DEBUG
 """"""""""""""""""
 
 ``LIBOMPTARGET_DEBUG`` controls whether or not debugging information will be
 displayed. This feature is only availible if ``libomptarget`` was built with
 ``-DOMPTARGET_DEBUG``. The debugging output provided is intended for use by
 ``libomptarget`` developers. More user-friendly output is presented when using

 runtime call.
 
 .. toctree::
    :hidden:
    :maxdepth: 1
 
    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``)
 -------------------------------------------------------------------
 
 .. _device_runtime:
 
 
 .. _remote_offloading_plugin:
 

openmp/libomptarget/include/device.h

public:
   uint64_t getDynRefCount() const { return States->DynRefCount; }
 
   /// Get the hold reference count.
   uint64_t getHoldRefCount() const { return States->HoldRefCount; }
 
   /// 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; }
 
   /// Reset the specified reference count unless it's infinity.  Reset to 1
   /// (even if currently 0) so it can be followed by a decrement.
   void resetRefCount(bool UseHoldRefCount) const {
     uint64_t &ThisRefCount =
         UseHoldRefCount ? States->HoldRefCount : States->DynRefCount;
uint64_t OtherRefCount =
         UseHoldRefCount ? States->DynRefCount : States->HoldRefCount;
     if (OtherRefCount > 0)
       return false;
     if (AfterReset)
       return ThisRefCount != INFRefCount;
     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(); }
 };
 
 typedef uintptr_t HstPtrBeginTy;
 inline bool operator<(const HostDataToTargetTy &lhs, const HstPtrBeginTy &rhs) {
   return lhs.HstPtrBegin < rhs;
 }
 inline bool operator<(const HstPtrBeginTy &lhs, const HostDataToTargetTy &rhs) {
   return lhs < rhs.HstPtrBegin;
 }
 inline bool operator<(const HostDataToTargetTy &lhs,
private:
   // Call to RTL
   void init(); // To be called only via DeviceTy::initOnce()
 };
 
 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;
 
   /// Devices associated with RTLs
   std::vector<std::unique_ptr<DeviceTy>> Devices;
   std::mutex RTLsMtx; ///< For RTLs and Devices
 
   /// Translation table retreived from the binary
   HostEntriesBeginToTransTableTy HostEntriesBeginToTransTable;
   std::mutex TrlTblMtx; ///< For Translation Table
   /// Host offload entries in order of image registration
   std::vector<__tgt_offload_entry *> HostEntriesBeginRegistrationOrder;
 
   /// Map from ptrs on the host to an entry in the Translation Table
   HostPtrToTableMapTy HostPtrToTableMap;
   std::mutex TblMapMtx; ///< For HostPtrToTableMap
 
   // 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;
 
 #endif

openmp/libomptarget/src/device.cpp

 #include "private.h"
 #include "rtl.h"
 
 #include <cassert>
 #include <climits>
 #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(),
       ShadowPtrMap(), DataMapMtx(), PendingGlobalsMtx(), ShadowMtx() {}
 
 DeviceTy::~DeviceTy() {
   if (DeviceID == -1 || !(getInfoLevel() & OMP_INFOTYPE_DUMP_TABLE))
     return;
if (LR.Flags.IsContained ||
     TargetPointer = (void *)Ptr;
   }
 
   // If the target pointer is valid, and we need to transfer data, issue the
   // data transfer.
   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();
 
     DP("Moving %" PRId64 " bytes (hst:" DPxMOD ") -> (tgt:" DPxMOD ")\n", Size,
        DPxPTR(HstPtrBegin), DPxPTR(TargetPointer));
 
     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();
     // If not a host pointer and no present modifier, we need to wait for the
     // event if it exists.
     // 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.
           REPORT("Failed to wait for event " DPxMOD ".\n", DPxPTR(Event));
           return {{false /* IsNewEntry */, false /* IsHostPointer */},
                   {} /* MapTableEntry */,
                   nullptr /* TargetPointer */};
         }
-      } else {
-        Entry->unlock();
       }
     }
   }
 
   return {{IsNew, IsHostPtr}, Entry, TargetPointer};
 }
 
 // Used by targetDataBegin, targetDataEnd, targetDataUpdate and target.

openmp/libomptarget/src/omptarget.cpp

if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ && !IsHostPtr) {
           Entry->second.TgtPtrVal != ExpectedTgtPtrBase) {
         // create or update shadow pointers for this entry
         Device.ShadowPtrMap[Pointer_HstPtrBegin] = {
             HstPtrBase, PointerTgtPtrBegin, ExpectedTgtPtrBase};
         UpdateDevPtr = true;
       }
 
       if (UpdateDevPtr) {
-        Pointer_TPR.MapTableEntry->lock();
+        HostDataToTargetTy::LockGuard LG(*Pointer_TPR.MapTableEntry);
         Device.ShadowMtx.unlock();
 
         DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",
            DPxPTR(PointerTgtPtrBegin), DPxPTR(TgtPtrBegin));
 
         void *&TgtPtrBase = AsyncInfo.getVoidPtrLocation();
         TgtPtrBase = ExpectedTgtPtrBase;
 
         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();
     }
   }
 
   return OFFLOAD_SUCCESS;
 }
 

openmp/libomptarget/src/rtl.cpp

 PluginManager *PM;
 
 #if OMPTARGET_PROFILE_ENABLED
 static char *ProfileTraceFile = nullptr;
 #endif
 
 __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");
   // TODO: add a configuration option for time granularity
   if (ProfileTraceFile)
     llvm::timeTraceProfilerInitialize(500 /* us */, "libomptarget");
 #endif
 }