[OpenMP] Separate Requirements into a standalone header

[jdoerfert]

This is not completely NFC since we now check all 4 requirements and the test is checking the good and the bad case for combining flags.

[llvmbot]

@llvm/pr-subscribers-openmp

Author: Johannes Doerfert (jdoerfert)

Changes

This is not completely NFC since we now check all 4 requirements and the test is checking the good and the bad case for combining flags.

---

Full diff: https://github.com/llvm/llvm-project/pull/74126.diff

8 Files Affected:

• (modified) openmp/libomptarget/include/PluginManager.h (+11-5)
• (added) openmp/libomptarget/include/Shared/Requirements.h (+88)
• (modified) openmp/libomptarget/include/omptarget.h (-15)
• (modified) openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h (+1)
• (modified) openmp/libomptarget/src/device.cpp (+3-3)
• (modified) openmp/libomptarget/src/interface.cpp (+1-1)
• (modified) openmp/libomptarget/src/rtl.cpp (-41)
• (modified) openmp/libomptarget/test/offloading/requires.c (+14-6)

diff --git a/openmp/libomptarget/include/PluginManager.h b/openmp/libomptarget/include/PluginManager.h
index 91298928716b6a8..5c7e13739664e4d 100644
--- a/openmp/libomptarget/include/PluginManager.h
+++ b/openmp/libomptarget/include/PluginManager.h
@@ -15,6 +15,7 @@
 
 #include "Shared/APITypes.h"
 #include "Shared/PluginAPI.h"
+#include "Shared/Requirements.h"
 
 #include "device.h"
 
@@ -23,6 +24,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/DynamicLibrary.h"
 
+#include <cstdint>
 #include <list>
 #include <mutex>
 #include <string>
@@ -66,13 +68,8 @@ struct PluginAdaptorTy {
 
 /// RTLs identified in the system.
 struct PluginAdaptorManagerTy {
-  int64_t RequiresFlags = OMP_REQ_UNDEFINED;
-
   explicit PluginAdaptorManagerTy() = default;
 
-  // Register the clauses of the requires directive.
-  void registerRequires(int64_t Flags);
-
   // Register a shared library with all (compatible) RTLs.
   void registerLib(__tgt_bin_desc *Desc);
 
@@ -148,12 +145,21 @@ struct PluginManager {
     return llvm::make_range(PluginAdaptors.begin(), PluginAdaptors.end());
   }
 
+  /// Return the user provided requirements.
+  int64_t getRequirements() const { return Requirements.getRequirements(); }
+
+  /// Add \p Flags to the user provided requirements.
+  void addRequirements(int64_t Flags) { Requirements.addRequirements(Flags); }
+
 private:
   bool RTLsLoaded = false;
   llvm::SmallVector<__tgt_bin_desc *> DelayedBinDesc;
 
   // List of all plugin adaptors, in use or not.
   std::list<PluginAdaptorTy> PluginAdaptors;
+
+  /// The user provided requirements.
+  RequirementCollection Requirements;
 };
 
 extern PluginManager *PM;
diff --git a/openmp/libomptarget/include/Shared/Requirements.h b/openmp/libomptarget/include/Shared/Requirements.h
new file mode 100644
index 000000000000000..19d6b8ffca495f4
--- /dev/null
+++ b/openmp/libomptarget/include/Shared/Requirements.h
@@ -0,0 +1,88 @@
+//===-- OpenMP/Requirements.h - User required requirements -----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Handling of the `omp requires` directive, e.g., requiring unified shared
+// memory.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef OMPTARGET_OPENMP_REQUIREMENTS_H
+#define OMPTARGET_OPENMP_REQUIREMENTS_H
+
+#include "Shared/Debug.h"
+
+#include "llvm/ADT/StringRef.h"
+
+#include <cassert>
+#include <cstdint>
+
+enum OpenMPOffloadingRequiresDirFlags : int64_t {
+  /// flag undefined.
+  OMP_REQ_UNDEFINED = 0x000,
+  /// no requires directive present.
+  OMP_REQ_NONE = 0x001,
+  /// reverse_offload clause.
+  OMP_REQ_REVERSE_OFFLOAD = 0x002,
+  /// unified_address clause.
+  OMP_REQ_UNIFIED_ADDRESS = 0x004,
+  /// unified_shared_memory clause.
+  OMP_REQ_UNIFIED_SHARED_MEMORY = 0x008,
+  /// dynamic_allocators clause.
+  OMP_REQ_DYNAMIC_ALLOCATORS = 0x010
+};
+
+class RequirementCollection {
+  int64_t SetFlags = OMP_REQ_UNDEFINED;
+
+  /// Check consistency between different requires flags (from different
+  /// translation units).
+  void checkConsistency(int64_t NewFlags, int64_t SetFlags,
+                        OpenMPOffloadingRequiresDirFlags Flag,
+                        llvm::StringRef Clause) {
+    if ((SetFlags & Flag) != (NewFlags & Flag)) {
+      FATAL_MESSAGE(2, "'#pragma omp requires %s' not used consistently!",
+                    Clause.data());
+    }
+  }
+
+public:
+  /// Register \p NewFlags as part of the user requirements.
+  void addRequirements(int64_t NewFlags) {
+    // TODO: add more elaborate check.
+    // Minimal check: only set requires flags if previous value
+    // is undefined. This ensures that only the first call to this
+    // function will set the requires flags. All subsequent calls
+    // will be checked for compatibility.
+    assert(NewFlags != OMP_REQ_UNDEFINED &&
+           "illegal undefined flag for requires directive!");
+    if (SetFlags == OMP_REQ_UNDEFINED) {
+      SetFlags = NewFlags;
+      return;
+    }
+
+    // If multiple compilation units are present enforce
+    // consistency across all of them for require clauses:
+    //  - reverse_offload
+    //  - unified_address
+    //  - unified_shared_memory
+    //  - dynamic_allocators
+    checkConsistency(NewFlags, SetFlags, OMP_REQ_REVERSE_OFFLOAD,
+                     "reverse_offload");
+    checkConsistency(NewFlags, SetFlags, OMP_REQ_UNIFIED_ADDRESS,
+                     "unified_address");
+    checkConsistency(NewFlags, SetFlags, OMP_REQ_UNIFIED_SHARED_MEMORY,
+                     "unified_shared_memory");
+    checkConsistency(NewFlags, SetFlags, OMP_REQ_DYNAMIC_ALLOCATORS,
+                     "dynamic_allocators");
+  }
+
+  /// Return the user provided requirements.
+  int64_t getRequirements() const { return SetFlags; }
+};
+
+#endif // OMPTARGET_OPENMP_DEVICE_REQUIREMENTS_H
diff --git a/openmp/libomptarget/include/omptarget.h b/openmp/libomptarget/include/omptarget.h
index 829e000f4fb3a4e..476a158019d3c5f 100644
--- a/openmp/libomptarget/include/omptarget.h
+++ b/openmp/libomptarget/include/omptarget.h
@@ -99,21 +99,6 @@ enum OpenMPOffloadingDeclareTargetFlags {
   OMP_DECLARE_TARGET_INDIRECT = 0x08
 };
 
-enum OpenMPOffloadingRequiresDirFlags {
-  /// flag undefined.
-  OMP_REQ_UNDEFINED               = 0x000,
-  /// no requires directive present.
-  OMP_REQ_NONE                    = 0x001,
-  /// reverse_offload clause.
-  OMP_REQ_REVERSE_OFFLOAD         = 0x002,
-  /// unified_address clause.
-  OMP_REQ_UNIFIED_ADDRESS         = 0x004,
-  /// unified_shared_memory clause.
-  OMP_REQ_UNIFIED_SHARED_MEMORY   = 0x008,
-  /// dynamic_allocators clause.
-  OMP_REQ_DYNAMIC_ALLOCATORS      = 0x010
-};
-
 enum TargetAllocTy : int32_t {
   TARGET_ALLOC_DEVICE = 0,
   TARGET_ALLOC_HOST,
diff --git a/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h b/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h
index dd601e6b4231aae..ab6c457fba78645 100644
--- a/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h
+++ b/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h
@@ -22,6 +22,7 @@
 #include "Shared/Debug.h"
 #include "Shared/Environment.h"
 #include "Shared/EnvironmentVar.h"
+#include "Shared/Requirements.h"
 #include "Shared/Utils.h"
 
 #include "GlobalHandler.h"
diff --git a/openmp/libomptarget/src/device.cpp b/openmp/libomptarget/src/device.cpp
index 31499cbf9317d1e..feb5d64190e5fec 100644
--- a/openmp/libomptarget/src/device.cpp
+++ b/openmp/libomptarget/src/device.cpp
@@ -281,7 +281,7 @@ TargetPointerResultTy DeviceTy::getTargetPointer(
       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 &&
+  } else if (PM->getRequirements() & 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
@@ -445,7 +445,7 @@ DeviceTy::getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool UpdateRefCount,
          LR.TPR.getEntry()->dynRefCountToStr().c_str(), DynRefCountAction,
          LR.TPR.getEntry()->holdRefCountToStr().c_str(), HoldRefCountAction);
     LR.TPR.TargetPointer = (void *)TP;
-  } else if (PM->RTLs.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) {
+  } else if (PM->getRequirements() & OMP_REQ_UNIFIED_SHARED_MEMORY) {
     // If the value isn't found in the mapping and unified shared memory
     // is on then it means we have stumbled upon a value which we need to
     // use directly from the host.
@@ -529,7 +529,7 @@ int DeviceTy::deallocTgtPtrAndEntry(HostDataToTargetTy *Entry, int64_t Size) {
 void DeviceTy::init() {
   // Make call to init_requires if it exists for this plugin.
   if (RTL->init_requires)
-    RTL->init_requires(PM->RTLs.RequiresFlags);
+    RTL->init_requires(PM->getRequirements());
   int32_t Ret = RTL->init_device(RTLDeviceID);
   if (Ret != OFFLOAD_SUCCESS)
     return;
diff --git a/openmp/libomptarget/src/interface.cpp b/openmp/libomptarget/src/interface.cpp
index 3d82b0250faed8a..2266dd039636bb4 100644
--- a/openmp/libomptarget/src/interface.cpp
+++ b/openmp/libomptarget/src/interface.cpp
@@ -39,7 +39,7 @@ using namespace llvm::omp::target::ompt;
 /// adds requires flags
 EXTERN void __tgt_register_requires(int64_t Flags) {
   TIMESCOPE();
-  PM->RTLs.registerRequires(Flags);
+  PM->addRequirements(Flags);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/openmp/libomptarget/src/rtl.cpp b/openmp/libomptarget/src/rtl.cpp
index 52ea76438d79a82..afe47d6145b7108 100644
--- a/openmp/libomptarget/src/rtl.cpp
+++ b/openmp/libomptarget/src/rtl.cpp
@@ -164,47 +164,6 @@ static __tgt_image_info getImageInfo(__tgt_device_image *Image) {
   return __tgt_image_info{(*BinaryOrErr)->getArch().data()};
 }
 
-void PluginAdaptorManagerTy::registerRequires(int64_t Flags) {
-  // TODO: add more elaborate check.
-  // Minimal check: only set requires flags if previous value
-  // is undefined. This ensures that only the first call to this
-  // function will set the requires flags. All subsequent calls
-  // will be checked for compatibility.
-  assert(Flags != OMP_REQ_UNDEFINED &&
-         "illegal undefined flag for requires directive!");
-  if (RequiresFlags == OMP_REQ_UNDEFINED) {
-    RequiresFlags = Flags;
-    return;
-  }
-
-  // If multiple compilation units are present enforce
-  // consistency across all of them for require clauses:
-  //  - reverse_offload
-  //  - unified_address
-  //  - unified_shared_memory
-  if ((RequiresFlags & OMP_REQ_REVERSE_OFFLOAD) !=
-      (Flags & OMP_REQ_REVERSE_OFFLOAD)) {
-    FATAL_MESSAGE0(
-        1, "'#pragma omp requires reverse_offload' not used consistently!");
-  }
-  if ((RequiresFlags & OMP_REQ_UNIFIED_ADDRESS) !=
-      (Flags & OMP_REQ_UNIFIED_ADDRESS)) {
-    FATAL_MESSAGE0(
-        1, "'#pragma omp requires unified_address' not used consistently!");
-  }
-  if ((RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) !=
-      (Flags & OMP_REQ_UNIFIED_SHARED_MEMORY)) {
-    FATAL_MESSAGE0(
-        1,
-        "'#pragma omp requires unified_shared_memory' not used consistently!");
-  }
-
-  // TODO: insert any other missing checks
-
-  DP("New requires flags %" PRId64 " compatible with existing %" PRId64 "!\n",
-     Flags, RequiresFlags);
-}
-
 void PluginAdaptorManagerTy::registerLib(__tgt_bin_desc *Desc) {
   PM->RTLsMtx.lock();
 
diff --git a/openmp/libomptarget/test/offloading/requires.c b/openmp/libomptarget/test/offloading/requires.c
index e5e58012a37cabd..cf01a73661d8f40 100644
--- a/openmp/libomptarget/test/offloading/requires.c
+++ b/openmp/libomptarget/test/offloading/requires.c
@@ -1,5 +1,7 @@
-// RUN: %libomptarget-compile-generic && env LIBOMPTARGET_DEBUG=1 %libomptarget-run-generic 2>&1 | %fcheck-generic -allow-empty -check-prefix=DEBUG
-// REQUIRES: libomptarget-debug
+// clang-format off
+// RUN: %libomptarget-compile-generic -DREQ=1 && %libomptarget-run-generic 2>&1 | %fcheck-generic -check-prefix=GOOD
+// RUN: %libomptarget-compile-generic -DREQ=2 && not %libomptarget-run-generic 2>&1 | %fcheck-generic -check-prefix=BAD
+// clang-format on
 
 /*
   Test for the 'requires' clause check.
@@ -24,11 +26,17 @@ void run_reg_requires() {
   // of the requires clauses. Since there are no requires clauses in this file
   // the flags state can only be OMP_REQ_NONE i.e. 1.
 
-  // This is the 2nd time this function is called so it should print the debug
-  // info belonging to the check.
+  // This is the 2nd time this function is called so it should print SUCCESS if
+  // REQ is compatible with `1` and otherwise cause an error.
   __tgt_register_requires(1);
-  __tgt_register_requires(1);
-  // DEBUG: New requires flags 1 compatible with existing 1!
+  __tgt_register_requires(REQ);
+
+  printf("SUCCESS");
+
+  // clang-format off
+  // GOOD: SUCCESS
+  // BAD: omptarget fatal error 2: '#pragma omp requires reverse_offload' not used consistently!
+  // clang-format on
 }
 
 // ---------------------------------------------------------------------------

[jhuber6]

I hate this runtime function.

[jdoerfert]

we should have globals in a section. But that's not for this cleanup to do.