HIP
HIP copied to clipboard
ROCm
`HIP_CLANG_INCLUDE_PATH` shouldn't be found from `../llvm/...` path
This logic:
https://github.com/ROCm-Developer-Tools/HIP/blob/1f53fbea8f52c76c9f9901f03b08d64f106ef098/hip-lang-config.cmake.in#L75-L79
doesn't work in Spack when clang is installed elsewhere.
Since CMake 3.21 identifies the compiler as ROCMClang now, can't you just get this path from CMAKE_HIP_COMPILER and stop searching for it?
For example relying on CMAKE_[lang]_IMPLICIT_INCLUDE_DIRECTORIES seems like a much better idea to me:
cmake_minimum_required(VERSION 3.21.0)
project(Hello)
if (CMAKE_CXX_COMPILER_ID STREQUAL "ROCMClang")
find_path(HIP_CLANG_INCLUDE_PATH __clang_cuda_math.h HINTS ${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES})
message("${HIP_CLANG_INCLUDE_PATH}")
endif()
$ CXX=hipcc cmake ..
-- The C compiler identification is GNU 9.3.0
-- The CXX compiler identification is ROCMClang 4.3.21334
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /tmp/tmp.3gxGnO8Zxs/.spack-env/view/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
/spack/opt/spack/linux-ubuntu20.04-zen2/gcc-10.3.0/llvm-amdgpu-4.3.0-ym3mboqa34lupzpgoy475aowqcpiazeo/lib/clang/13.0.0/include
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/tmp.3gxGnO8Zxs/hello/build
Do either of these open CMake issues describe your problem, or is this something different?
- https://gitlab.kitware.com/cmake/cmake/-/issues/22457
- https://gitlab.kitware.com/cmake/cmake/-/issues/22536
Sorry @zackgalbreath, I didn't see the notification for your reply.
But no, it's orthogonal.
The problem here is that you shouldn't expect llvm to be in ../llvm. It can be anywhere. You should find it from clang, not from the hip install.
This is now biting RAJA under spack in production at LLNL. Even after the reorg the cmake code is still looking for llvm at a static offset from hip.
@haampie Thanks for the suggestion. We had evaluated the suggested change. Looks like cmake support for HIP compiler as ROCMClang has been reverted. Please refer https://gitlab.kitware.com/cmake/cmake/-/blob/master/Help/release/3.21.rst for more details. Though we cant take the proposed change as it is, but are evaluating other options. Will update as soon as possible.
Alternate approach to find clang path directly (independent of hip install and relative path) is addressed in https://github.com/ROCm-Developer-Tools/HIP/pull/2964. Please take a look.
Alternate approach to find clang path directly (independent of hip install and relative path) is addressed in https://github.com/ROCm-Developer-Tools/HIP/pull/2964.
That is the wrong approach. The hip clang path should be found relative to the CMAKE_HIP_COMPILER. You should not do find_dependency(LLVM) as this might find a different llvm than the one being used by CMAKE_HIP_COMPILER.
Or more to the point amdclang should actually install a cmake package that can be found, with find_dependency. Why would it be beneficial to search for it relative to the compiler, when the compiler may or may not be amdclang, and may or may not actually be in a specific relative path from the headers? That would break on the top level bin directory outside the llvm directory without extra work, this came up recently because other cmake logic in ROCM already broke that way.
Why would it be beneficial to search for it relative to the compiler
These are private headers used by the compiler. There is no guarantee of compatibility between different versions of compilers. That is why we need to use the header that correspond to the compiler being used. Calling find_dependency could find headers from a different compiler which could be incompatible.
when the compiler may or may not be amdclang
The compiler has to be set to a compiler supporting hip, which means these header would be provided.
That would break on the top level bin directory outside the llvm directory without extra work
So the current approach of using "${_IMPORT_PREFIX}/../llvm/lib/clang/*/include" is wrong, since this is relative to hip runtime and not the compiler. I am not sure if that is what you are referring to. What I am saying is that we should be doing something like "${CMAKE_HIP_COMPILER}/../lib/clang/*/include"(we will actually want to get the real path after any symlinks though).
Furthermore, there maybe flags we could print out from the compiler to get this information. Something like that was done in 2964 to get the compiler_rt lib, so there might be something for the includes as well. This should be more robust than globbing files.
Yes, using llvm-config or a flag on the compiler would be substantially better than any globbing or path manipulation. This however: "${CMAKE_HIP_COMPILER}/../lib/clang/*/include" is still not always right, and that's what I was saying above. Yes, it needs to be a compiler that supports HIP, which right now is either amdclang, hipcc or cray cpe. The amdclang++ that people usually grab is the symlink at <rocm-path>/bin/amdclang++, doing the path translation above will fail to find the appropriate path from there. Doing realpath or similar on it first could work around that in the simple case at least, but it could equally well be a wrapper script which would break things again.
That said, getting the paths from the compiler or its associated configuration driver like llvm-config is a great idea, if that's an option that seems like the way to go for sure.
Yes, it needs to be a compiler that supports HIP, which right now is either amdclang, hipcc or cray cpe.
Not just amdclang, but any recent versions of upstream clang support hip as well. However, hipcc is not a compiler just a wrapper script, and I am not familiar with what cray cpe is.
For hipcc, we actually use the installed path that is printed out from --version for find_package(hip):
https://github.com/ROCm-Developer-Tools/hipamd/blob/develop/hip-config.cmake.in#L165
We could probably do the same thing for HIP language.
getting the paths from the compiler
This would probably simplify a lot what we are doing. Ideally, it would be better if the compiler just added these paths to private headers automatically when using -x hip.
its associated configuration driver like llvm-config
Trying to find an associated llvm-config will be the same problem as finding associated headers, so this wouldn't help at all.
Yeah, using HIP_CLANG_ROOT like the hipcc logic does would probably work. I completely agree about having them in path with -x hip, that seems like a simple driver change to include the appropriate -isystem and it would make this whole thing much nicer.
I'm aware hipcc is a wrapper script, I'd love to see that cease to exist, but CPE is cray's (or rather HPE's) HPC compiler stack built on top of clang. It supports HIP, and in fact supports combining HIP with their OpenMP offload as part of the compiler strategy for El Capitan and Frontier. It's supported as a CMAKE_HIP_COMPILER value, but does not specify a normal clang++ in a normal path for that value, but the path to the CPE driver. That's why I mentioned it, using the value of CMAKE_HIP_COMPILER is dangerous as a starting point.
Finding llvm-config is not an issue though, since <clang-cmd> -print-prog-name=llvm-config will print the full path to it.
That said, honestly it might make sense to just build on the builtin -print-resource-dir and go from there, that gets the equivalent of <clang-path>/../lib/clang/<ver>/, as in:
❯ clang -print-resource-dir
/usr/lib/llvm-14/lib/clang/14.0.0
clang currently can set the proper include path by itself for HIP. It no longer needs hipcc or cmake to specify clang include path.
clang currently can set the proper include path by itself for HIP.
You mean clang will set the include path for the private headers it uses? If so, then we can just remove the flags for clang's private headers and libraries(ie compiler-rt).
However, clang cant set the path for HIP runtime correctly because it doesnt know where HIP is installed. Especially for source-based package managers like spack(and others) this is installed in a completely different directory than the compiler because they are seperate sources and builds. So we still need cmake to set the flags for HIP's includes and libraries(and perhaps device libs if those are still seperate from llvm).
clang is able to set the include path for its private headers. Currently, it still cannot add the library for compiler-rt.
It can set the options for HIP include path and HIP library if HIP is installed in the default location. Otherwise, it cannot.
As per the comments, proposed new changes https://github.com/ROCm-Developer-Tools/HIP/pull/3124 for addressing the issue discussed. Kindly take a look. Thanks.
I have yet to test that change, but it looks like it flat removes HIP_CLANG_INCLUDE_PATH, was that not necessary to begin with?
I have yet to test that change, but it looks like it flat removes HIP_CLANG_INCLUDE_PATH, was that not necessary to begin with? Yes as part of the change, we have removed detection of HIP_CLANG_INCLUDE_PATH and use of it , since clang will add include path for itself.