[Openmp-commits] [openmp] [OpenMP] Separate Requirements into a standalone header (PR #74126)
Johannes Doerfert via Openmp-commits
openmp-commits at lists.llvm.org
Fri Dec 1 10:58:28 PST 2023
https://github.com/jdoerfert created https://github.com/llvm/llvm-project/pull/74126
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.
>From d70d1cb89f305a881c609191000b580a9f08a28a Mon Sep 17 00:00:00 2001
From: Johannes Doerfert <johannes at jdoerfert.de>
Date: Thu, 30 Nov 2023 20:54:19 -0800
Subject: [PATCH] [OpenMP] Separate Requirements into a standalone header
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.
---
openmp/libomptarget/include/PluginManager.h | 16 ++--
.../include/Shared/Requirements.h | 88 +++++++++++++++++++
openmp/libomptarget/include/omptarget.h | 15 ----
.../common/include/PluginInterface.h | 1 +
openmp/libomptarget/src/device.cpp | 6 +-
openmp/libomptarget/src/interface.cpp | 2 +-
openmp/libomptarget/src/rtl.cpp | 41 ---------
.../libomptarget/test/offloading/requires.c | 20 +++--
8 files changed, 118 insertions(+), 71 deletions(-)
create mode 100644 openmp/libomptarget/include/Shared/Requirements.h
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
}
// ---------------------------------------------------------------------------
More information about the Openmp-commits
mailing list