release/18.x: [OpenMP][OMPT] Fix hsa include when building amdgpu/src/rtl.cpp #95484
Conversation
…/rtl.cpp The dynamic_hsa/ include directory is required by both optional dynamic_hsa/hsa.cpp and non-optional src/rtl.cpp. It should then always be included or the build will fail if only src/rtl.cpp is built. This also simplifies the way header files from dynamic_hsa/ are included in src/rtl.cpp. Fixes: error: ‘HSA_AMD_AGENT_INFO_TIMESTAMP_FREQUENCY’ was not declared in this scope
|
@llvm/pr-subscribers-backend-amdgpu Author: Thomas Debesse (illwieckz) ChangesThe It should then always be included or the build will fail if only This also simplifies the way header files from Fixes: Full diff: https://github.com/llvm/llvm-project/pull/95484.diff 2 Files Affected:
diff --git a/openmp/libomptarget/plugins-nextgen/amdgpu/CMakeLists.txt b/openmp/libomptarget/plugins-nextgen/amdgpu/CMakeLists.txt
index 68ce63467a6c8..42cc560c79112 100644
--- a/openmp/libomptarget/plugins-nextgen/amdgpu/CMakeLists.txt
+++ b/openmp/libomptarget/plugins-nextgen/amdgpu/CMakeLists.txt
@@ -38,13 +38,15 @@ add_definitions(-DDEBUG_PREFIX="TARGET AMDGPU RTL")
set(LIBOMPTARGET_DLOPEN_LIBHSA OFF)
option(LIBOMPTARGET_FORCE_DLOPEN_LIBHSA "Build with dlopened libhsa" ${LIBOMPTARGET_DLOPEN_LIBHSA})
+# Required by both optional dynamic_hsa/hsa.cpp and non-optional src/rtl.cpp.
+include_directories(dynamic_hsa)
+
if (${hsa-runtime64_FOUND} AND NOT LIBOMPTARGET_FORCE_DLOPEN_LIBHSA)
libomptarget_say("Building AMDGPU NextGen plugin linked against libhsa")
set(LIBOMPTARGET_EXTRA_SOURCE)
set(LIBOMPTARGET_DEP_LIBRARIES hsa-runtime64::hsa-runtime64)
else()
libomptarget_say("Building AMDGPU NextGen plugin for dlopened libhsa")
- include_directories(dynamic_hsa)
set(LIBOMPTARGET_EXTRA_SOURCE dynamic_hsa/hsa.cpp)
set(LIBOMPTARGET_DEP_LIBRARIES)
endif()
diff --git a/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp b/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
index 81634ae1edc49..8cedc72d5f63c 100644
--- a/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
+++ b/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
@@ -56,18 +56,8 @@
#define BIGENDIAN_CPU
#endif
-#if defined(__has_include)
-#if __has_include("hsa/hsa.h")
-#include "hsa/hsa.h"
-#include "hsa/hsa_ext_amd.h"
-#elif __has_include("hsa.h")
#include "hsa.h"
#include "hsa_ext_amd.h"
-#endif
-#else
-#include "hsa/hsa.h"
-#include "hsa/hsa_ext_amd.h"
-#endif
namespace llvm {
namespace omp {
|
|
I first noticed the issue when building the chipStar fork of LLVM 17: https://github.com/CHIP-SPV/llvm-project (the The whole folder disappeared in It would be good to backport it to LLVM 17 too. Should I make another PR targetting I haven't checked it yet if versions older than LLVM 17 are affected. |
|
This is different from this file right? I'm trying to fix an issue when building LLVM 14 with a newer ROCm releases which fails to find the newer |
|
The 14 branch seems to be very old, espially the file you link is in |
|
@pranav-sivaraman try this patch: diff --git a/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt b/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt
index 92523c23f68b..92bcd94edb7a 100644
--- a/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt
+++ b/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt
@@ -56,13 +56,14 @@ include_directories(
set(LIBOMPTARGET_DLOPEN_LIBHSA OFF)
option(LIBOMPTARGET_FORCE_DLOPEN_LIBHSA "Build with dlopened libhsa" ${LIBOMPTARGET_DLOPEN_LIBHSA})
+include_directories(dynamic_hsa)
+
if (${hsa-runtime64_FOUND} AND NOT LIBOMPTARGET_FORCE_DLOPEN_LIBHSA)
libomptarget_say("Building AMDGPU plugin linked against libhsa")
set(LIBOMPTARGET_EXTRA_SOURCE)
set(LIBOMPTARGET_DEP_LIBRARIES hsa-runtime64::hsa-runtime64)
else()
libomptarget_say("Building AMDGPU plugin for dlopened libhsa")
- include_directories(dynamic_hsa)
set(LIBOMPTARGET_EXTRA_SOURCE dynamic_hsa/hsa.cpp)
set(LIBOMPTARGET_DEP_LIBRARIES)
endif()I haven't tested it, but maybe the mistake is similar. |
There was a problem hiding this comment.
We made a change recently that made the dynamic_hsa version the default. The error you're seeing is from an old HSA, so if you're overriding the default to use an old library that's probably not worth working around.
Actually, this is for the release/18.x? Maybe worth hacking around it in that case since that's older.
The error I see comes from the fact there is no old HSA around to workaround an LLVM bug. There is no The Without such patch, LLVM requires ROCm to be installed and configured to be in default includes for This patch is to make LLVM use This patch is needed to build both I assume a full LLVM build will not trigger the build problem because something else will include |
The This symbol is expected to be provided by The code in |
|
Here is a script to reproduce the bug: #! /usr/bin/env bash
set -x -u -e -o pipefail
version="${1:-18}"
CMAKE_BUILD_PARALLEL_LEVEL="$(nproc)"
export CMAKE_BUILD_PARALLEL_LEVEL="${CMAKE_BUILD_PARALLEL_LEVEL:-4}"
workspace="llvm-bug95484-${version}"
rm -rf "${workspace}"
mkdir "${workspace}"
cd "${workspace}"
git clone --depth 1 \
--branch "release/${version}.x" \
'https://github.com/llvm/llvm-project.git' \
'llvm-project'
git clone --depth 1 \
'https://github.com/KhronosGroup/SPIRV-Headers.git' \
'llvm-project/llvm/projects/SPIRV-Headers'
git clone --depth 1 \
--branch "llvm_release_${version}0" \
'https://github.com/KhronosGroup/SPIRV-LLVM-Translator.git' \
'llvm-project/llvm/projects/SPIRV-LLVM-Translator'
cmake \
-S'llvm-project/llvm' \
-B'build' \
-G'Ninja' \
-D'CMAKE_INSTALL_PREFIX'='install' \
-D'CMAKE_BUILD_TYPE'='Release' \
-D'LLVM_ENABLE_PROJECTS'='clang;openmp' \
-D'LLVM_TARGETS_TO_BUILD'='Native' \
-D'LLVM_EXPERIMENTAL_TARGETS_TO_BUILD'='SPIRV' \
-D'LLVM_EXTERNAL_PROJECTS'='SPIRV-Headers'
cmake --build 'build'
cmake --install 'build'It can be used just by saving it as
It will fail this way: Edit: The script probably builds useless things, but this is the use case I know that reproduces the bug. |
|
I reproduce the bug with both I don't reproduce the bug with I cannot test |
This was introduced in ROCm-5.3, see https://github.com/ROCm/ROCR-Runtime/blob/rocm-5.3.x/src/inc/hsa_ext_amd.h#L333. The Using |
|
If we make being declare variant elide on a user defined compile time condition, we could use the change in the It's not possible right now but not hard to do. |
Why is it expected for LLVM to build against system HSA first, then to rely on LLVM's HSA only if system one is missing? I'm building LLVM, not something else, so I expect LLVM to build against LLVM files.
If LLVM was attempting to use LLVM's HSA first, it would be possible to do If a two-weeks old ROCm 6.1.2 is already considered too old by both LLVM 17, it is safe to assume that ROCm 6.1.2 will never be young enough for LLVM17… Then I don't see why both LLVM 17 and LLVM 18 would try to use something that will never be young enough… |
|
Setting |
|
And the ROCm HSA also provides |
|
I discovered something wrong: It looks like I have some HSA 5.2.3 files… The This So ROCm 6.1.2 actually installs both This doesn't make sense, the ROCm packages may be just badly made by AMD. When I temporarily remove With So it looks like I'm tracking a ROCm packaging bug from AMD side. But I still don't get why LLVM doesn't use its own HSA by default and doesn't use its own headers for enum values used in its own code. |
I don't think AMD handles the actual packaging, that's on the maintainers for your distribution. I'm guessing that the HSA runtime is a separate package from ROCm and gets put somewhere else for whatever reason. I don't have any issues like that with my distribution.
It does in the main branch, but I think you're right that the logic causes it to look at the system one first since |
They do: https://repo.radeon.com It happens that Ubuntu provides an old 5.2.3
Maybe, instead of adding #if defined(__has_include)
#if __has_include("dynamic_hsa/hsa.h")
#include "dynamic_hsa/hsa.h"
#include "dynamic_hsa/hsa_ext_amd.h"
#if __has_include("hsa/hsa.h")
#include "hsa/hsa.h"
#include "hsa/hsa_ext_amd.h"
#elif __has_include("hsa.h")
#include "hsa.h"
#include "hsa_ext_amd.h"
#endif
#else
#include "dynamic_hsa/hsa.h"
#include "dynamic_hsa/hsa_ext_amd.h"
#endif |
|
The So basically: This is definitely an AMD packaging issue. |
Summary: The HSA headers existed previously in `include/hsa.h` and were moved to `include/hsa/hsa.h` in a later ROCm version. The include headers here were originally designed to favor a newer one. However, this unintentionally prevented the dyanmic HSA's `hsa.h` from being used if both were present. This patch changes the order so it will be found first. Related to llvm#95484.
|
Made #95769, don't know if the window for the 18.x window is still open for a back port. |
Summary: The HSA headers existed previously in `include/hsa.h` and were moved to `include/hsa/hsa.h` in a later ROCm version. The include headers here were originally designed to favor a newer one. However, this unintentionally prevented the dyanmic HSA's `hsa.h` from being used if both were present. This patch changes the order so it will be found first. Related to #95484.
|
I merged my patch, I think the window to backport to 18.x is passed so you'll need to use the main branch for now. Thanks for bringing this to my attention. |
The
dynamic_hsa/include directory is required by both optionaldynamic_hsa/hsa.cppand non-optionalsrc/rtl.cpp.It should then always be included or the build will fail if only
src/rtl.cppis built.This also simplifies the way header files from
dynamic_hsa/are included insrc/rtl.cpp.Fixes: