Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Offloading][NFC] Move creation of offloading entries from OpenMP #70116

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Oct 24, 2023

Summary:
This patch is a first step to remove dependencies on the OpenMPIRBuilder
for creating generic offloading entries. This patch changes no
functionality and merely moves the code around. In the future the
interface will be changed to allow for more code re-use in the
registration and creation of offloading entries as well as a more
generic interface for CUDA, HIP, OpenMP, and SYCL(?). Doing this as a
first step to reduce the noise involved in the functional changes.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen flang:openmp clang:openmp OpenMP related changes to Clang labels Oct 24, 2023
@llvmbot
Copy link

llvmbot commented Oct 24, 2023

@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Joseph Huber (jhuber6)

Changes

Summary:
This patch is a first step to remove dependencies on the OpenMPIRBuilder
for creating generic offloading entires. This patch changes no
functionality and merely moves the code around. In the future the
interface will be changed to allow for more code re-use in the
registeration and creation of offloading entries as well as a more
generic interface for CUDA, HIP, OpenMP, and SYCL(?). Doing this as a
first step to reduce the noise involved in the functional changes.


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

10 Files Affected:

  • (modified) clang/lib/CodeGen/CGCUDANV.cpp (+6-7)
  • (modified) clang/lib/CodeGen/CMakeLists.txt (+1)
  • (added) llvm/include/llvm/Frontend/Offloading/Utility.h (+37)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h (-21)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMPKinds.def (-2)
  • (modified) llvm/lib/Frontend/CMakeLists.txt (+1)
  • (added) llvm/lib/Frontend/Offloading/CMakeLists.txt (+14)
  • (added) llvm/lib/Frontend/Offloading/Utility.cpp (+76)
  • (modified) llvm/lib/Frontend/OpenMP/CMakeLists.txt (+1)
  • (modified) llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp (+3-39)
diff --git a/clang/lib/CodeGen/CGCUDANV.cpp b/clang/lib/CodeGen/CGCUDANV.cpp
index 8a1212f2272e87a..a8b9c1325731aa6 100644
--- a/clang/lib/CodeGen/CGCUDANV.cpp
+++ b/clang/lib/CodeGen/CGCUDANV.cpp
@@ -19,6 +19,7 @@
 #include "clang/Basic/Cuda.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
 #include "clang/CodeGen/ConstantInitBuilder.h"
+#include "llvm/Frontend/Offloading/Utility.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
@@ -1129,13 +1130,11 @@ void CGNVCUDARuntime::transformManagedVars() {
 // registered. The linker will provide a pointer to this section so we can
 // register the symbols with the linked device image.
 void CGNVCUDARuntime::createOffloadingEntries() {
-  llvm::OpenMPIRBuilder OMPBuilder(CGM.getModule());
-  OMPBuilder.initialize();
-
   StringRef Section = CGM.getLangOpts().HIP ? "hip_offloading_entries"
                                             : "cuda_offloading_entries";
+  llvm::Module &M = CGM.getModule();
   for (KernelInfo &I : EmittedKernels)
-    OMPBuilder.emitOffloadingEntry(KernelHandles[I.Kernel->getName()],
+    llvm::offloading::emitOffloadingEntry(M, KernelHandles[I.Kernel->getName()],
                                    getDeviceSideName(cast<NamedDecl>(I.D)), 0,
                                    DeviceVarFlags::OffloadGlobalEntry, Section);
 
@@ -1143,17 +1142,17 @@ void CGNVCUDARuntime::createOffloadingEntries() {
     uint64_t VarSize =
         CGM.getDataLayout().getTypeAllocSize(I.Var->getValueType());
     if (I.Flags.getKind() == DeviceVarFlags::Variable) {
-      OMPBuilder.emitOffloadingEntry(
+      llvm::offloading::emitOffloadingEntry(M, 
           I.Var, getDeviceSideName(I.D), VarSize,
           I.Flags.isManaged() ? DeviceVarFlags::OffloadGlobalManagedEntry
                               : DeviceVarFlags::OffloadGlobalEntry,
           Section);
     } else if (I.Flags.getKind() == DeviceVarFlags::Surface) {
-      OMPBuilder.emitOffloadingEntry(I.Var, getDeviceSideName(I.D), VarSize,
+      llvm::offloading::emitOffloadingEntry(M, I.Var, getDeviceSideName(I.D), VarSize,
                                      DeviceVarFlags::OffloadGlobalSurfaceEntry,
                                      Section);
     } else if (I.Flags.getKind() == DeviceVarFlags::Texture) {
-      OMPBuilder.emitOffloadingEntry(I.Var, getDeviceSideName(I.D), VarSize,
+      llvm::offloading::emitOffloadingEntry(M, I.Var, getDeviceSideName(I.D), VarSize,
                                      DeviceVarFlags::OffloadGlobalTextureEntry,
                                      Section);
     }
diff --git a/clang/lib/CodeGen/CMakeLists.txt b/clang/lib/CodeGen/CMakeLists.txt
index d67ce982d78acf3..da98848e3b44387 100644
--- a/clang/lib/CodeGen/CMakeLists.txt
+++ b/clang/lib/CodeGen/CMakeLists.txt
@@ -11,6 +11,7 @@ set(LLVM_LINK_COMPONENTS
   Extensions
   FrontendHLSL
   FrontendOpenMP
+  FrontendOffloading
   HIPStdPar
   IPO
   IRPrinter
diff --git a/llvm/include/llvm/Frontend/Offloading/Utility.h b/llvm/include/llvm/Frontend/Offloading/Utility.h
new file mode 100644
index 000000000000000..f74f9e3ff119fd8
--- /dev/null
+++ b/llvm/include/llvm/Frontend/Offloading/Utility.h
@@ -0,0 +1,37 @@
+//===- Utility.h - Collection of geneirc offloading utilities -------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/Module.h"
+#include "llvm/Object/OffloadBinary.h"
+
+namespace llvm {
+namespace offloading {
+
+/// Create an offloading section struct used to register this global at
+/// runtime.
+///
+/// Type struct __tgt_offload_entry{
+///   void    *addr;      // Pointer to the offload entry info.
+///                       // (function or global)
+///   char    *name;      // Name of the function or global.
+///   size_t  size;       // Size of the entry info (0 if it a function).
+///   int32_t flags;
+///   int32_t reserved;
+/// };
+///
+/// \param M The module to be used
+/// \param Addr The pointer to the global being registered.
+/// \param Name The symbol name associated with the global.
+/// \param Size The size in bytes of the global (0 for functions).
+/// \param Flags Flags associated with the entry.
+/// \param SectionName The section this entry will be placed at.
+void emitOffloadingEntry(Module &M, Constant *Addr, StringRef Name,
+                         uint64_t Size, int32_t Flags, StringRef SectionName);
+
+} // namespace offloading
+} // namespace llvm
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
index 00b4707a7f820d7..c2cfdfd32324dbf 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -1358,27 +1358,6 @@ class OpenMPIRBuilder {
   /// Value.
   GlobalValue *createGlobalFlag(unsigned Value, StringRef Name);
 
-  /// Create an offloading section struct used to register this global at
-  /// runtime.
-  ///
-  /// Type struct __tgt_offload_entry{
-  ///   void    *addr;      // Pointer to the offload entry info.
-  ///                       // (function or global)
-  ///   char    *name;      // Name of the function or global.
-  ///   size_t  size;       // Size of the entry info (0 if it a function).
-  ///   int32_t flags;
-  ///   int32_t reserved;
-  /// };
-  ///
-  /// \param Addr The pointer to the global being registered.
-  /// \param Name The symbol name associated with the global.
-  /// \param Size The size in bytes of the global (0 for functions).
-  /// \param Flags Flags associated with the entry.
-  /// \param SectionName The section this entry will be placed at.
-  void emitOffloadingEntry(Constant *Addr, StringRef Name, uint64_t Size,
-                           int32_t Flags,
-                           StringRef SectionName = "omp_offloading_entries");
-
   /// Generate control flow and cleanup for cancellation.
   ///
   /// \param CancelFlag Flag indicating if the cancellation is performed.
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def b/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
index 4823c4cc6b833ec..39780e7c8934cf1 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -88,8 +88,6 @@ __OMP_ARRAY_TYPE(Int32Arr3, Int32, 3)
   OMP_STRUCT_TYPE(VarName, "struct." #Name, Packed, __VA_ARGS__)
 
 __OMP_STRUCT_TYPE(Ident, ident_t, false, Int32, Int32, Int32, Int32, Int8Ptr)
-__OMP_STRUCT_TYPE(OffloadEntry, __tgt_offload_entry, false, Int8Ptr, Int8Ptr, SizeTy,
-                  Int32, Int32)
 __OMP_STRUCT_TYPE(KernelArgs, __tgt_kernel_arguments, false, Int32, Int32, VoidPtrPtr,
 		  VoidPtrPtr, Int64Ptr, Int64Ptr, VoidPtrPtr, VoidPtrPtr,
 		  Int64, Int64, Int32Arr3Ty, Int32Arr3Ty, Int32)
diff --git a/llvm/lib/Frontend/CMakeLists.txt b/llvm/lib/Frontend/CMakeLists.txt
index fa48c975a8b3e5a..5ef092e0a1a9ede 100644
--- a/llvm/lib/Frontend/CMakeLists.txt
+++ b/llvm/lib/Frontend/CMakeLists.txt
@@ -1,3 +1,4 @@
 add_subdirectory(HLSL)
 add_subdirectory(OpenACC)
 add_subdirectory(OpenMP)
+add_subdirectory(Offloading)
diff --git a/llvm/lib/Frontend/Offloading/CMakeLists.txt b/llvm/lib/Frontend/Offloading/CMakeLists.txt
new file mode 100644
index 000000000000000..25eb24785732edf
--- /dev/null
+++ b/llvm/lib/Frontend/Offloading/CMakeLists.txt
@@ -0,0 +1,14 @@
+add_llvm_component_library(LLVMFrontendOffloading
+  Utility.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend
+
+  DEPENDS
+  intrinsics_gen
+
+  LINK_COMPONENTS
+  Core
+  Support
+  TransformUtils
+)
diff --git a/llvm/lib/Frontend/Offloading/Utility.cpp b/llvm/lib/Frontend/Offloading/Utility.cpp
new file mode 100644
index 000000000000000..e676ea27ca9f9e8
--- /dev/null
+++ b/llvm/lib/Frontend/Offloading/Utility.cpp
@@ -0,0 +1,76 @@
+//===- Utility.cpp ------ Collection of geneirc offloading utilities ------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Frontend/Offloading/Utility.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalValue.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/Value.h"
+
+using namespace llvm;
+using namespace llvm::offloading;
+
+static IntegerType *getSizeTTy(Module &M) {
+  LLVMContext &C = M.getContext();
+  switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+  case 4u:
+    return Type::getInt32Ty(C);
+  case 8u:
+    return Type::getInt64Ty(C);
+  }
+  llvm_unreachable("unsupported pointer type size");
+}
+
+// TODO: Export this to the linker wrapper code registration.
+static StructType *getEntryTy(Module &M) {
+  LLVMContext &C = M.getContext();
+  StructType *EntryTy = StructType::getTypeByName(C, "__tgt_offload_entry");
+  if (!EntryTy)
+    EntryTy = StructType::create("__tgt_offload_entry", Type::getInt8PtrTy(C),
+                                 Type::getInt8PtrTy(C), getSizeTTy(M),
+                                 Type::getInt32Ty(C), Type::getInt32Ty(C));
+  return EntryTy;
+}
+
+// TODO: Rework this interface to be more generic.
+void offloading::emitOffloadingEntry(Module &M, Constant *Addr, StringRef Name,
+                                     uint64_t Size, int32_t Flags,
+                                     StringRef SectionName) {
+  Type *Int8PtrTy = Type::getInt8PtrTy(M.getContext());
+  Type *Int32Ty = Type::getInt32Ty(M.getContext());
+  Type *SizeTy = M.getDataLayout().getIntPtrType(M.getContext());
+
+  Constant *AddrName = ConstantDataArray::getString(M.getContext(), Name);
+
+  // Create the constant string used to look up the symbol in the device.
+  auto *Str =
+      new llvm::GlobalVariable(M, AddrName->getType(), /*isConstant=*/true,
+                               llvm::GlobalValue::InternalLinkage, AddrName,
+                               ".omp_offloading.entry_name");
+  Str->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
+  // Construct the offloading entry.
+  Constant *EntryData[] = {
+      ConstantExpr::getPointerBitCastOrAddrSpaceCast(Addr, Int8PtrTy),
+      ConstantExpr::getPointerBitCastOrAddrSpaceCast(Str, Int8PtrTy),
+      ConstantInt::get(SizeTy, Size),
+      ConstantInt::get(Int32Ty, Flags),
+      ConstantInt::get(Int32Ty, 0),
+  };
+  Constant *EntryInitializer = ConstantStruct::get(getEntryTy(M), EntryData);
+
+  auto *Entry = new GlobalVariable(
+      M, getEntryTy(M),
+      /*isConstant=*/true, GlobalValue::WeakAnyLinkage, EntryInitializer,
+      ".omp_offloading.entry." + Name, nullptr, GlobalValue::NotThreadLocal,
+      M.getDataLayout().getDefaultGlobalsAddressSpace());
+
+  // The entry has to be created in the section the linker expects it to be.
+  Entry->setSection(SectionName);
+  Entry->setAlignment(Align(1));
+}
diff --git a/llvm/lib/Frontend/OpenMP/CMakeLists.txt b/llvm/lib/Frontend/OpenMP/CMakeLists.txt
index a2eedabc3ed6903..67aedf5c2b61a5c 100644
--- a/llvm/lib/Frontend/OpenMP/CMakeLists.txt
+++ b/llvm/lib/Frontend/OpenMP/CMakeLists.txt
@@ -20,4 +20,5 @@ add_llvm_component_library(LLVMFrontendOpenMP
   MC
   Scalar
   BitReader
+  FrontendOffloading
   )
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 5b24e9fe2e0c5bd..d765d2bd7b14a30 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -13,6 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/Frontend/Offloading/Utility.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
@@ -958,44 +959,6 @@ OpenMPIRBuilder::createCancel(const LocationDescription &Loc,
   return Builder.saveIP();
 }
 
-void OpenMPIRBuilder::emitOffloadingEntry(Constant *Addr, StringRef Name,
-                                          uint64_t Size, int32_t Flags,
-                                          StringRef SectionName) {
-  Type *Int8PtrTy = Type::getInt8PtrTy(M.getContext());
-  Type *Int32Ty = Type::getInt32Ty(M.getContext());
-  Type *SizeTy = M.getDataLayout().getIntPtrType(M.getContext());
-
-  Constant *AddrName = ConstantDataArray::getString(M.getContext(), Name);
-
-  // Create the constant string used to look up the symbol in the device.
-  auto *Str =
-      new llvm::GlobalVariable(M, AddrName->getType(), /*isConstant=*/true,
-                               llvm::GlobalValue::InternalLinkage, AddrName,
-                               ".omp_offloading.entry_name");
-  Str->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
-
-  // Construct the offloading entry.
-  Constant *EntryData[] = {
-      ConstantExpr::getPointerBitCastOrAddrSpaceCast(Addr, Int8PtrTy),
-      ConstantExpr::getPointerBitCastOrAddrSpaceCast(Str, Int8PtrTy),
-      ConstantInt::get(SizeTy, Size),
-      ConstantInt::get(Int32Ty, Flags),
-      ConstantInt::get(Int32Ty, 0),
-  };
-  Constant *EntryInitializer =
-      ConstantStruct::get(OpenMPIRBuilder::OffloadEntry, EntryData);
-
-  auto *Entry = new GlobalVariable(
-      M, OpenMPIRBuilder::OffloadEntry,
-      /* isConstant = */ true, GlobalValue::WeakAnyLinkage, EntryInitializer,
-      ".omp_offloading.entry." + Name, nullptr, GlobalValue::NotThreadLocal,
-      M.getDataLayout().getDefaultGlobalsAddressSpace());
-
-  // The entry has to be created in the section the linker expects it to be.
-  Entry->setSection(SectionName);
-  Entry->setAlignment(Align(1));
-}
-
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitTargetKernel(
     const LocationDescription &Loc, InsertPointTy AllocaIP, Value *&Return,
     Value *Ident, Value *DeviceID, Value *NumTeams, Value *NumThreads,
@@ -5928,7 +5891,8 @@ void OpenMPIRBuilder::createOffloadEntry(Constant *ID, Constant *Addr,
                                          GlobalValue::LinkageTypes,
                                          StringRef Name) {
   if (!Config.isGPU()) {
-    emitOffloadingEntry(ID, Name.empty() ? Addr->getName() : Name, Size, Flags);
+    llvm::offloading::emitOffloadingEntry(M, ID, Name.empty() ? Addr->getName()
+        : Name, Size, Flags, "omp_offloading_entries");
     return;
   }
   // TODO: Add support for global variables on the device after declare target

@github-actions
Copy link

github-actions bot commented Oct 24, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@jhuber6 jhuber6 force-pushed the MoveEntries branch 3 times, most recently from 2af5fc3 to 8add211 Compare October 24, 2023 23:30
@shiltian
Copy link
Contributor

Can this stuff really be generic?

@jhuber6
Copy link
Contributor Author

jhuber6 commented Oct 24, 2023

Can this stuff really be generic?

Obviously flags are going to be target dependent, but the actual form, generation, and iteration of these can be generic. The idea is to put them all into a single section such that we can filter out the ones that apply to whatever runtime so they can all live in the same section and re-use all the same code.

@arsenm
Copy link
Contributor

arsenm commented Oct 25, 2023

to allow for more code re-use in the registeration

Typo 'registeration'

Summary:
This patch is a first step to remove dependencies on the OpenMPIRBuilder
for creating generic offloading entires. This patch changes no
functionality and merely moves the code around. In the future the
interface will be changed to allow for more code re-use in the
registeration and creation of offloading entries as well as a more
generic interface for CUDA, HIP, OpenMP, and SYCL(?). Doing this as a
first step to reduce the noise involved in the functional changes.
@yxsamliu
Copy link
Collaborator

Maybe a clang documentation can be added for this generic offloading toolchain for OpenMP/CUDA/HIP. Could be a brief description about the offloading entries and registration mechanism in the beginning then buffy it up later.

@jhuber6
Copy link
Contributor Author

jhuber6 commented Oct 25, 2023

Maybe a clang documentation can be added for this generic offloading toolchain for OpenMP/CUDA/HIP. Could be a brief description about the offloading entries and registration mechanism in the beginning then buffy it up later.

We have https://clang.llvm.org/docs/OffloadingDesign.html which describes the entries, but from an OpenMP perspective. But I have thought about reworking it once I finish up the new driver support (which this is a part of).

Copy link
Collaborator

@yxsamliu yxsamliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@jhuber6 jhuber6 merged commit 078ae8c into llvm:main Oct 25, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category flang:openmp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants