[Offload] Do not link every target for JIT #92013

jhuber6 · 2024-05-13T19:17:04Z

Summary:
The offload library supports basic JIT functionality, however we
currently link against every single target even though only AMDGPU and
NVPTX are supported. This somewhat bloats the dynamic library list, so
we should constrain it to what's actually used.

llvmbot · 2024-05-13T19:17:36Z

@llvm/pr-subscribers-backend-amdgpu

Author: Joseph Huber (jhuber6)

Changes

Summary:
The offload library supports basic JIT functionality, however we
currently link against every single target even though only AMDGPU and
NVPTX are supported. This somewhat bloats the dynamic library list, so
we should constrain it to what's actually used.

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

4 Files Affected:

(modified) offload/plugins-nextgen/CMakeLists.txt (+2-2)
(modified) offload/plugins-nextgen/amdgpu/CMakeLists.txt (+5-1)
(modified) offload/plugins-nextgen/cuda/CMakeLists.txt (+5-1)
(modified) offload/plugins-nextgen/host/CMakeLists.txt (+1-1)

diff --git a/offload/plugins-nextgen/CMakeLists.txt b/offload/plugins-nextgen/CMakeLists.txt
index d1079f8a3e9cc..b8426b7816518 100644
--- a/offload/plugins-nextgen/CMakeLists.txt
+++ b/offload/plugins-nextgen/CMakeLists.txt
@@ -13,10 +13,10 @@
 # Common interface to handle creating a plugin library.
 set(common_dir ${CMAKE_CURRENT_SOURCE_DIR}/common)
 add_subdirectory(common)
-function(add_target_library target_name lib_name)
+function(add_target_library target_name lib_name llvm_components)
   add_llvm_library(${target_name} STATIC
     LINK_COMPONENTS
-      ${LLVM_TARGETS_TO_BUILD}
+      ${llvm_components}
       AggressiveInstCombine
       Analysis
       BinaryFormat
diff --git a/offload/plugins-nextgen/amdgpu/CMakeLists.txt b/offload/plugins-nextgen/amdgpu/CMakeLists.txt
index 738183f8945ed..968532907b28c 100644
--- a/offload/plugins-nextgen/amdgpu/CMakeLists.txt
+++ b/offload/plugins-nextgen/amdgpu/CMakeLists.txt
@@ -28,7 +28,11 @@ if(NOT (CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(ppc64le)|(aarch64)$" AND CMAKE
 endif()
 
 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.amdgpu AMDGPU)
+if("AMDGPU" IN_LIST LLVM_TARGETS_TO_BUILD)
+  add_target_library(omptarget.rtl.amdgpu AMDGPU AMDGPU)
+else()
+  add_target_library(omptarget.rtl.amdgpu AMDGPU "")
+endif()
 
 target_sources(omptarget.rtl.amdgpu PRIVATE src/rtl.cpp)
 target_include_directories(omptarget.rtl.amdgpu PRIVATE
diff --git a/offload/plugins-nextgen/cuda/CMakeLists.txt b/offload/plugins-nextgen/cuda/CMakeLists.txt
index dd684bb223431..d56eaa43c374d 100644
--- a/offload/plugins-nextgen/cuda/CMakeLists.txt
+++ b/offload/plugins-nextgen/cuda/CMakeLists.txt
@@ -24,7 +24,11 @@ endif()
 libomptarget_say("Building CUDA NextGen offloading plugin.")
 
 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.cuda CUDA)
+if("NVPTX" IN_LIST LLVM_TARGETS_TO_BUILD)
+  add_target_library(omptarget.rtl.cuda CUDA NVPTX)
+else()
+  add_target_library(omptarget.rtl.cuda CUDA "")
+endif()
 
 target_sources(omptarget.rtl.cuda PRIVATE src/rtl.cpp)
 
diff --git a/offload/plugins-nextgen/host/CMakeLists.txt b/offload/plugins-nextgen/host/CMakeLists.txt
index 72b5681283fe2..92f2677e8a855 100644
--- a/offload/plugins-nextgen/host/CMakeLists.txt
+++ b/offload/plugins-nextgen/host/CMakeLists.txt
@@ -14,7 +14,7 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "ppc64le$")
 endif()
 
 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.host ${machine})
+add_target_library(omptarget.rtl.host ${machine} "")
 
 target_sources(omptarget.rtl.host PRIVATE src/rtl.cpp)

llvmbot · 2024-05-13T19:17:36Z

@llvm/pr-subscribers-offload

Author: Joseph Huber (jhuber6)

Changes

Summary:
The offload library supports basic JIT functionality, however we
currently link against every single target even though only AMDGPU and
NVPTX are supported. This somewhat bloats the dynamic library list, so
we should constrain it to what's actually used.

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

4 Files Affected:

(modified) offload/plugins-nextgen/CMakeLists.txt (+2-2)
(modified) offload/plugins-nextgen/amdgpu/CMakeLists.txt (+5-1)
(modified) offload/plugins-nextgen/cuda/CMakeLists.txt (+5-1)
(modified) offload/plugins-nextgen/host/CMakeLists.txt (+1-1)

diff --git a/offload/plugins-nextgen/CMakeLists.txt b/offload/plugins-nextgen/CMakeLists.txt
index d1079f8a3e9cc..b8426b7816518 100644
--- a/offload/plugins-nextgen/CMakeLists.txt
+++ b/offload/plugins-nextgen/CMakeLists.txt
@@ -13,10 +13,10 @@
 # Common interface to handle creating a plugin library.
 set(common_dir ${CMAKE_CURRENT_SOURCE_DIR}/common)
 add_subdirectory(common)
-function(add_target_library target_name lib_name)
+function(add_target_library target_name lib_name llvm_components)
   add_llvm_library(${target_name} STATIC
     LINK_COMPONENTS
-      ${LLVM_TARGETS_TO_BUILD}
+      ${llvm_components}
       AggressiveInstCombine
       Analysis
       BinaryFormat
diff --git a/offload/plugins-nextgen/amdgpu/CMakeLists.txt b/offload/plugins-nextgen/amdgpu/CMakeLists.txt
index 738183f8945ed..968532907b28c 100644
--- a/offload/plugins-nextgen/amdgpu/CMakeLists.txt
+++ b/offload/plugins-nextgen/amdgpu/CMakeLists.txt
@@ -28,7 +28,11 @@ if(NOT (CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(ppc64le)|(aarch64)$" AND CMAKE
 endif()
 
 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.amdgpu AMDGPU)
+if("AMDGPU" IN_LIST LLVM_TARGETS_TO_BUILD)
+  add_target_library(omptarget.rtl.amdgpu AMDGPU AMDGPU)
+else()
+  add_target_library(omptarget.rtl.amdgpu AMDGPU "")
+endif()
 
 target_sources(omptarget.rtl.amdgpu PRIVATE src/rtl.cpp)
 target_include_directories(omptarget.rtl.amdgpu PRIVATE
diff --git a/offload/plugins-nextgen/cuda/CMakeLists.txt b/offload/plugins-nextgen/cuda/CMakeLists.txt
index dd684bb223431..d56eaa43c374d 100644
--- a/offload/plugins-nextgen/cuda/CMakeLists.txt
+++ b/offload/plugins-nextgen/cuda/CMakeLists.txt
@@ -24,7 +24,11 @@ endif()
 libomptarget_say("Building CUDA NextGen offloading plugin.")
 
 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.cuda CUDA)
+if("NVPTX" IN_LIST LLVM_TARGETS_TO_BUILD)
+  add_target_library(omptarget.rtl.cuda CUDA NVPTX)
+else()
+  add_target_library(omptarget.rtl.cuda CUDA "")
+endif()
 
 target_sources(omptarget.rtl.cuda PRIVATE src/rtl.cpp)
 
diff --git a/offload/plugins-nextgen/host/CMakeLists.txt b/offload/plugins-nextgen/host/CMakeLists.txt
index 72b5681283fe2..92f2677e8a855 100644
--- a/offload/plugins-nextgen/host/CMakeLists.txt
+++ b/offload/plugins-nextgen/host/CMakeLists.txt
@@ -14,7 +14,7 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "ppc64le$")
 endif()
 
 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.host ${machine})
+add_target_library(omptarget.rtl.host ${machine} "")
 
 target_sources(omptarget.rtl.host PRIVATE src/rtl.cpp)

shiltian · 2024-05-13T20:15:02Z

offload/plugins-nextgen/amdgpu/CMakeLists.txt

@@ -28,7 +28,11 @@ if(NOT (CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(ppc64le)|(aarch64)$" AND CMAKE
 endif()

 # Create the library and add the default arguments.
-add_target_library(omptarget.rtl.amdgpu AMDGPU)
+if("AMDGPU" IN_LIST LLVM_TARGETS_TO_BUILD)


Why don't we put this logic into add_target_library?

Because we shouldn't link against AMDGPU if the user didn't request to build the amdgpu plugin.

I'd make this same as the argument LINK_COMPONENTS of add_llvm_library, since it is not always required. Besides, given add_target_library is only for plugin, and only "host" plugin doesn't require that, we can assume almost all plugins need the corresponding target. add_target_library(omptarget.rtl.amdgpu AMDGPU "") isn't really beautiful. We can even force lib_name to be the target name.

Honestly we should probably rename the amdgpu plugin to hsa anyway? Since the important distinction is the runtime, not the target architecture. Potentially in the future we'll have things like OpenCL or Vulkan I guess? Which are supposed to be generic in their own right.
So, what we could do is just target_library(libomptarget.amdgpu AMDGPU) or something, but I think that might require something else.

shiltian · 2024-05-14T03:21:01Z

You could potentially do this in offload/plugins-nextgen/common/CMakeLists.txt:

foreach(Target ${TargetsSupported})
	target_compile_definitions(PluginCommon PRIVATE "LIBOMPTARGET_JIT_${Target}")
        llvm_map_components_to_libnames(llvm_libs ${Target})
        target_link_libraries(PluginCommon INTERFACE ${llvm_libs})
endforeach()

In this way, there is no need for changes of each target. Technically it should have been done on PluginCommon since this is the CMake target that needs those symbols.

jhuber6 · 2024-05-14T03:25:48Z

You could potentially do this in offload/plugins-nextgen/common/CMakeLists.txt:
foreach(Target ${TargetsSupported})
	target_compile_definitions(PluginCommon PRIVATE "LIBOMPTARGET_JIT_${Target}")
        llvm_map_components_to_libnames(llvm_libs ${Target})
        target_link_libraries(PluginCommon INTERFACE ${llvm_libs})
endforeach()
In this way, there is no need for changes of each target. Technically it should have been done on PluginCommon since this is the CMake target that needs those symbols.

Thanks for pointing that out, this logic should be updated as well.

I want to do this in the plugins individually because we shouldn't link libomptarget against the AMDGPU backend if no one builds the amdgpu plugin.

shiltian · 2024-05-14T03:31:58Z

I want to do this in the plugins individually because we shouldn't link libomptarget against the AMDGPU backend if no one builds the amdgpu plugin.

But this doesn't work as your expectation because the AMDGPU code is there when you turn the macro on, no matter whether you build it for AMDGPU or NVPTX.

If you want to do that, one solution would be to add JIT.cpp as target source if the target is in support list when you build the corresponding plugin, instead of having it as a common component.

jhuber6 · 2024-05-14T16:03:52Z

But this doesn't work as your expectation because the AMDGPU code is there when you turn the macro on, no matter whether you build it for AMDGPU or NVPTX.

If you want to do that, one solution would be to add JIT.cpp as target source if the target is in support list when you build the corresponding plugin, instead of having it as a common component.

I suppose that's fair. It's easier to just put it there than it is to rework that bit.

Summary: The offload library supports basic JIT functionality, however we currently link against every single target even though only AMDGPU and NVPTX are supported. This somewhat bloats the dynamic library list, so we should constrain it to what's actually used.

shiltian · 2024-05-14T16:11:51Z

The problem with this patch is, you will end up with undefined symbols if you just link the corresponding target. For example, the LLVM was built with target AMDGPU and NVPTX. So when PluginCommon is compiled, the code for initializing both are enabled. However, when you eventually link AMDGPU plugin, you only link against AMDGPU. That will leave those NVPTX initialization code undefined symbols.

jhuber6 · 2024-05-14T16:34:54Z

The problem with this patch is, you will end up with undefined symbols if you just link the corresponding target. For example, the LLVM was built with target AMDGPU and NVPTX. So when PluginCommon is compiled, the code for initializing both are enabled. However, when you eventually link AMDGPU plugin, you only link against AMDGPU. That will leave those NVPTX initialization code undefined symbols.

I don't understand, it builds perfectly fine after a clean build?

shiltian · 2024-05-14T18:54:46Z

I don't understand, it builds perfectly fine after a clean build?

Hmm, I was referring to the previous version. The current one looks good.

jhuber6 requested review from jdoerfert, saiislam and shiltian May 13, 2024 19:17

llvmbot added backend:AMDGPU offload labels May 13, 2024

shiltian reviewed May 13, 2024

View reviewed changes

jhuber6 force-pushed the Target branch from bbcce81 to 6173d71 Compare May 14, 2024 02:53

jhuber6 force-pushed the Target branch from 6173d71 to bd677a5 Compare May 14, 2024 03:25

jhuber6 requested a review from jplehr May 14, 2024 16:02

jhuber6 force-pushed the Target branch from bd677a5 to 3e90b34 Compare May 14, 2024 16:10

shiltian approved these changes May 14, 2024

View reviewed changes

jhuber6 merged commit 302db1a into llvm:main May 14, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Offload] Do not link every target for JIT #92013

[Offload] Do not link every target for JIT #92013

jhuber6 commented May 13, 2024

llvmbot commented May 13, 2024

llvmbot commented May 13, 2024

shiltian May 13, 2024

jhuber6 May 13, 2024

shiltian May 13, 2024

jhuber6 May 13, 2024

shiltian commented May 14, 2024

jhuber6 commented May 14, 2024

shiltian commented May 14, 2024 •

edited

Loading

jhuber6 commented May 14, 2024

shiltian commented May 14, 2024

jhuber6 commented May 14, 2024

shiltian commented May 14, 2024

[Offload] Do not link every target for JIT #92013

[Offload] Do not link every target for JIT #92013

Conversation

jhuber6 commented May 13, 2024

llvmbot commented May 13, 2024

llvmbot commented May 13, 2024

shiltian May 13, 2024

Choose a reason for hiding this comment

jhuber6 May 13, 2024

Choose a reason for hiding this comment

shiltian May 13, 2024

Choose a reason for hiding this comment

jhuber6 May 13, 2024

Choose a reason for hiding this comment

shiltian commented May 14, 2024

jhuber6 commented May 14, 2024

shiltian commented May 14, 2024 • edited Loading

jhuber6 commented May 14, 2024

shiltian commented May 14, 2024

jhuber6 commented May 14, 2024

shiltian commented May 14, 2024

shiltian commented May 14, 2024 •

edited

Loading