[OpenMP] Remove dependency on LLVM include directory from DeviceRTL #136359

jhuber6 · 2025-04-18T19:38:01Z

Summary:
Currently we depend on a single LLVM include directory. This is actually
only required to define one enum, which is highly unlikely to change.
THis patch makes the Environment.h include directory more hermetic so
we no long depend on other libraries. In exchange, we get a simpler
dependency list for the price of hard-coding 1 somewhere. I think it's
a valid trade considering that this flag is highly unlikely to change at
this point.

@ronlieb AMD version https://gist.github.com/jhuber6/3313e6f957be14dc79fe85e5126d2cb3

llvmbot · 2025-04-18T19:38:38Z

@llvm/pr-subscribers-offload

Author: Joseph Huber (jhuber6)

Changes

Summary:
Currently we depend on a single LLVM include directory. This is actually
only required to define one enum, which is highly unlikely to change.
THis patch makes the Environment.h include directory more hermetic so
we no long depend on other libraries. In exchange, we get a simpler
dependency list for the price of hard-coding 1 somewhere. I think it's
a valid trade considering that this flag is highly unlikely to change at
this point.

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

4 Files Affected:

(modified) offload/DeviceRTL/CMakeLists.txt (-6)
(modified) offload/DeviceRTL/src/Kernel.cpp (+1-4)
(modified) offload/DeviceRTL/src/Mapping.cpp (-2)
(modified) offload/include/Shared/Environment.h (+3-9)

diff --git a/offload/DeviceRTL/CMakeLists.txt b/offload/DeviceRTL/CMakeLists.txt
index 2da2ade94f4e4..cce360236960f 100644
--- a/offload/DeviceRTL/CMakeLists.txt
+++ b/offload/DeviceRTL/CMakeLists.txt
@@ -75,10 +75,6 @@ if(${LIBOMPTARGET_GPU_LIBC_SUPPORT})
   list(APPEND clang_opt_flags -DOMPTARGET_HAS_LIBC)
 endif()
 
-# Prepend -I to each list element
-set (LIBOMPTARGET_LLVM_INCLUDE_DIRS_DEVICERTL "${LIBOMPTARGET_LLVM_INCLUDE_DIRS}")
-list(TRANSFORM LIBOMPTARGET_LLVM_INCLUDE_DIRS_DEVICERTL PREPEND "-I")
-
 # Set flags for LLVM Bitcode compilation.
 set(bc_flags -c -flto -std=c++17 -fvisibility=hidden
              ${clang_opt_flags} -nogpulib -nostdlibinc
@@ -88,7 +84,6 @@ set(bc_flags -c -flto -std=c++17 -fvisibility=hidden
              -I${include_directory}
              -I${devicertl_base_directory}/../include
              -I${devicertl_base_directory}/../../libc
-             ${LIBOMPTARGET_LLVM_INCLUDE_DIRS_DEVICERTL}
 )
 
 # first create an object target
@@ -172,7 +167,6 @@ function(compileDeviceRTLLibrary target_name target_triple)
       ${include_directory}
       ${devicertl_base_directory}/../../libc
       ${devicertl_base_directory}/../include
-      ${LIBOMPTARGET_LLVM_INCLUDE_DIRS}
     )
     install(TARGETS ${ide_target_name} EXCLUDE_FROM_ALL)
   endif()
diff --git a/offload/DeviceRTL/src/Kernel.cpp b/offload/DeviceRTL/src/Kernel.cpp
index 9bb89573dc0cb..32d3b6cf4b75a 100644
--- a/offload/DeviceRTL/src/Kernel.cpp
+++ b/offload/DeviceRTL/src/Kernel.cpp
@@ -21,8 +21,6 @@
 #include "Synchronization.h"
 #include "Workshare.h"
 
-#include "llvm/Frontend/OpenMP/OMPDeviceConstants.h"
-
 using namespace ompx;
 
 static void
@@ -74,8 +72,7 @@ extern "C" {
 int32_t __kmpc_target_init(KernelEnvironmentTy &KernelEnvironment,
                            KernelLaunchEnvironmentTy &KernelLaunchEnvironment) {
   ConfigurationEnvironmentTy &Configuration = KernelEnvironment.Configuration;
-  bool IsSPMD = Configuration.ExecMode &
-                llvm::omp::OMPTgtExecModeFlags::OMP_TGT_EXEC_MODE_SPMD;
+  bool IsSPMD = Configuration.ExecMode & /*OMP_TGT_EXEC_MODE_SPMD=*/1;
   bool UseGenericStateMachine = Configuration.UseGenericStateMachine;
   if (IsSPMD) {
     inititializeRuntime(/*IsSPMD=*/true, KernelEnvironment,
diff --git a/offload/DeviceRTL/src/Mapping.cpp b/offload/DeviceRTL/src/Mapping.cpp
index e951556c2ad4e..b145892d1ece0 100644
--- a/offload/DeviceRTL/src/Mapping.cpp
+++ b/offload/DeviceRTL/src/Mapping.cpp
@@ -16,8 +16,6 @@
 #include "State.h"
 #include "gpuintrin.h"
 
-#include "llvm/Frontend/OpenMP/OMPGridValues.h"
-
 using namespace ompx;
 
 // FIXME: This resolves the handling for the AMDGPU workgroup size when the ABI
diff --git a/offload/include/Shared/Environment.h b/offload/include/Shared/Environment.h
index db8443a7be933..2f943eaefa0f4 100644
--- a/offload/include/Shared/Environment.h
+++ b/offload/include/Shared/Environment.h
@@ -15,15 +15,9 @@
 
 #include <stdint.h>
 
-#ifdef OMPTARGET_DEVICE_RUNTIME
-#include "DeviceTypes.h"
-#else
-#include "SourceInfo.h"
+struct IdentTy;
 
-using IdentTy = ident_t;
-#endif
-
-#include "llvm/Frontend/OpenMP/OMPDeviceConstants.h"
+using OMPTgtExecModeFlags = unsigned char;
 
 enum class DeviceDebugKind : uint32_t {
   Assertion = 1U << 0,
@@ -80,7 +74,7 @@ struct DynamicEnvironmentTy {
 struct ConfigurationEnvironmentTy {
   uint8_t UseGenericStateMachine = 2;
   uint8_t MayUseNestedParallelism = 2;
-  llvm::omp::OMPTgtExecModeFlags ExecMode = llvm::omp::OMP_TGT_EXEC_MODE_SPMD;
+  OMPTgtExecModeFlags ExecMode;
   // Information about (legal) launch configurations.
   //{
   int32_t MinThreads = -1;

kparzysz · 2025-04-19T15:24:18Z

This is a bad idea. Why do you want to remove that dependency so bad?

jhuber6 · 2025-04-19T15:52:02Z

Because I want to put this back in openmp/ and it'd be the only dependency on LLVM. I don't want to need to find LLVM just for this one constant.

shiltian · 2025-04-21T15:50:30Z

In the past if I remember correctly, we need more values from LLVM headers for AMDGPU, such as the wavefront size. It seems like as we evolve recently, we only need one value. It might be "overkill" to include it.

If we move it back to openmp/, it will not be built by default right? A lot of libomp users build libomp out of tree. With that being said, even if we move it back, we can probably still have it depend on LLVM headers if the device runtime is built?

jhuber6 · 2025-04-21T15:52:40Z

In the past if I remember correctly, we need more values from LLVM headers for AMDGPU, such as the wavefront size. It seems like as we evolve recently, we only need one value. It might be "overkill" to include it.

If we move it back to openmp/, it will not be built by default right? A lot of libomp users build libomp out of tree. With that being said, even if we move it back, we can probably still have it depend on LLVM headers if the device runtime is built?

The idea is that it will build this version if you compile with the respective GPU triple, and the CPU library otherwise. So, if you enter in CMAKE_<LANG>_COMPILER_TARGET=amdgcn-amd-amdhsa for your standalone build, you get this. I could keep the include, but this just feels much cleaner since it's a single constant that is unlikely to ever change, and there's a comment for it.

shiltian · 2025-04-21T16:02:33Z

How are you gonna compile the entire openmp/ project with the GPU triple?

jhuber6 · 2025-04-21T16:04:03Z

How are you gonna compile the entire openmp/ project with the GPU triple?

if ("${TRIPLE}" MATCHES "^nvptx64|^amdgcn")
  add_subdirectory(device)
else()
  add_subdirectory(runtime)
endif()

shiltian · 2025-04-21T16:07:23Z

That's not going to completely work, right? If someone builds openmp/ out-of-tree using GCC as the compiler and sets CMAKE_CXX_COMPILER_TARGET to amdgcn, it's going to break.

jhuber6 · 2025-04-21T16:08:31Z

That's not going to completely work, right? If someone builds openmp/ out-of-tree using GCC as the compiler and sets CMAKE_CXX_COMPILER_TARGET to amdgcn, it's going to break.

I don't see how that's an issue. We already require that the device RTL is built with a 'just-built' clang that we pull out of the user's path. If anything, this lets you build 'cpu offloading' with a full GCC toolchain if you so desired, because right now it'll error without clang.

shiltian · 2025-04-21T16:16:41Z

We already require that the device RTL is built with a 'just-built' clang that we pull out of the user's path.

If that is the case, to depend on LLVM headers is not an issue?

jhuber6 · 2025-04-21T16:20:41Z

We already require that the device RTL is built with a 'just-built' clang that we pull out of the user's path.

If that is the case, to depend on LLVM headers is not an issue?

It's not an issue conceptually, we could definitely make it work, but there's no reason to make openmp/ depend on LLVM headers just for a single constant 1 value. That's a lot of searching for user paths and setting up include directories that we don't need to do. That's separate from the compiler. This is not solving a fundamental issue, it's just solving the fact that I don't want to need to copy a million lines of CMake back into openmp just to find a single header that does #define 1 basically.

kparzysz · 2025-04-21T17:19:51Z

but there's no reason to make openmp/ depend on LLVM headers just for a single constant 1 value.

There is definitely a reason, and that's to avoid hardcoding things.

There are precedents where definitions of certain enumerations are duplicated. You could follow that---copy/paste the definition of the one that's introducing the dependence with a comment that it must be in sync with the one in LLVM.

Summary: Currently we depend on a single LLVM include directory. This is actually only required to define one enum, which is highly unlikely to change. THis patch makes the `Environment.h` include directory more hermetic so we no long depend on other libraries. In exchange, we get a simpler dependency list for the price of hard-coding `1` somewhere. I think it's a valid trade considering that this flag is highly unlikely to change at this point.

jhuber6 · 2025-04-21T18:30:43Z

but there's no reason to make openmp/ depend on LLVM headers just for a single constant 1 value.

There is definitely a reason, and that's to avoid hardcoding things.

There are precedents where definitions of certain enumerations are duplicated. You could follow that---copy/paste the definition of the one that's introducing the dependence with a comment that it must be in sync with the one in LLVM.

Updated, better?

shiltian

LGTM as long as @kparzysz is fine with this.

kparzysz

LGTM, thanks!

jhuber6 · 2025-04-21T20:21:28Z

@ronlieb AMD version https://gist.github.com/jhuber6/3313e6f957be14dc79fe85e5126d2cb3

@ronlieb

…lvm#136359) Summary: Currently we depend on a single LLVM include directory. This is actually only required to define one enum, which is highly unlikely to change. THis patch makes the `Environment.h` include directory more hermetic so we no long depend on other libraries. In exchange, we get a simpler dependency list for the price of hard-coding `1` somewhere. I think it's a valid trade considering that this flag is highly unlikely to change at this point. @ronlieb AMD version https://gist.github.com/jhuber6/3313e6f957be14dc79fe85e5126d2cb3

@ronlieb

…lvm#136359) Summary: Currently we depend on a single LLVM include directory. This is actually only required to define one enum, which is highly unlikely to change. THis patch makes the `Environment.h` include directory more hermetic so we no long depend on other libraries. In exchange, we get a simpler dependency list for the price of hard-coding `1` somewhere. I think it's a valid trade considering that this flag is highly unlikely to change at this point. @ronlieb AMD version https://gist.github.com/jhuber6/3313e6f957be14dc79fe85e5126d2cb3

@ronlieb

…lvm#136359) Summary: Currently we depend on a single LLVM include directory. This is actually only required to define one enum, which is highly unlikely to change. THis patch makes the `Environment.h` include directory more hermetic so we no long depend on other libraries. In exchange, we get a simpler dependency list for the price of hard-coding `1` somewhere. I think it's a valid trade considering that this flag is highly unlikely to change at this point. @ronlieb AMD version https://gist.github.com/jhuber6/3313e6f957be14dc79fe85e5126d2cb3

jhuber6 requested review from jdoerfert, JonChesterfield, jplehr, mjklemm, ronlieb and shiltian April 18, 2025 19:38

llvmbot added the offload label Apr 18, 2025

jhuber6 force-pushed the RemoveLLVMInclude branch from 43c9c19 to 1e0d978 Compare April 21, 2025 18:30

shiltian approved these changes Apr 21, 2025

View reviewed changes

kparzysz approved these changes Apr 21, 2025

View reviewed changes

jhuber6 merged commit 56bf0e7 into llvm:main Apr 21, 2025
9 checks passed

[OpenMP] Remove dependency on LLVM include directory from DeviceRTL #136359

[OpenMP] Remove dependency on LLVM include directory from DeviceRTL #136359

Uh oh!

Conversation

jhuber6 commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

kparzysz commented Apr 19, 2025

Uh oh!

jhuber6 commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiltian commented Apr 21, 2025

Uh oh!

jhuber6 commented Apr 21, 2025

Uh oh!

shiltian commented Apr 21, 2025

Uh oh!

jhuber6 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiltian commented Apr 21, 2025

Uh oh!

jhuber6 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiltian commented Apr 21, 2025

Uh oh!

jhuber6 commented Apr 21, 2025

Uh oh!

kparzysz commented Apr 21, 2025

Uh oh!

jhuber6 commented Apr 21, 2025

Uh oh!

shiltian left a comment

Choose a reason for hiding this comment

Uh oh!

kparzysz left a comment

Choose a reason for hiding this comment

Uh oh!

jhuber6 commented Apr 21, 2025

Uh oh!

Uh oh!

Uh oh!

jhuber6 commented Apr 18, 2025 •

edited

Loading

jhuber6 commented Apr 19, 2025 •

edited

Loading

jhuber6 commented Apr 21, 2025 •

edited

Loading

jhuber6 commented Apr 21, 2025 •

edited

Loading