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

Manually check version of clang if ROCm is used. #800

Merged

Conversation

fodinabor
Copy link
Collaborator

This adds simplistic ROCm version discovery by relying on Clang's version output and adds an exemplary check for ROCm < 5 to enable HAS_TYPED_PTR. This should fix #797 and enable handling more issues more gracefully.

This assumes, that ROCm's Clang is built from a git branch called roc-([0-9]+).([0-9]+).([0-9]+).
An alternative approach would be to first (somehow) detect whether we have a ROCm clang and then parsing the output ${CLANG_EXECUTABLE_PATH} "-v" "-x" "hip" "--rocm-path=${ROCM_PATH}" "/dev/null" for the string Found HIP installation: .+, version ([0-9]+).([0-9]+).([0-9]+).
Maybe we can detect whether we have AMD clang more easily from CMake? If not, we could rely on it reporting AMD clang version .., which has to have been set using -DCLANG_VENDOR="AMD clang" during the LLVM build, though, which might not be the case if ROCm's llvm is built manually?

I was only able to verify any of those commands' stability with ROCm 5.2. Maybe someone can chip in with testing 4.5 and if it fails reporting the output of those two commands:

$ROCM_PATH/llvm/bin/clang --version
$ROCM_PATH/llvm/bin/clang -v -x hip < /dev/null |& grep HIP 

@al42and could you check with 4.5.2?

This assumes, that ROCm's Clang is built from a git branch called `roc-Major-Minor-Patch`.
An alternative approach would be to first (somehow) detect whether we have a ROCm clang and then parsing the output `${CLANG_EXECUTABLE_PATH} "-v" "-x" "hip" "--rocm-path=${ROCM_PATH}" "/dev/null"` for the string `Found HIP installation: .+, version ([0-9]+).([0-9]+).([0-9]+)`.
Maybe we can detect whether we have AMD clang more easily from CMake? If not, we could rely on it reporting `AMD clang version ..`, which has to have been set using `-DCLANG_VENDOR="AMD clang"` during the LLVM build, though, which might not be the case if ROCm's llvm is built manually?

Add exemplary check for ROCm < 5 to enable `HAS_TYPED_PTR`.
CMakeLists.txt Show resolved Hide resolved
@al42and
Copy link
Contributor

al42and commented Aug 8, 2022

Version detection works fine with 4.2.0, 4.3.0, and 4.5.2.

Does not work with 3.9.0 and 4.1.0 (clang does not print anything to differentiate it from upstream), but those versions are ancient, so whatever.

Would've been nice if hipSYCL were to auto-apply the HIPSYCL_NO_DEVICE_MANGLER fix in this case (see my comment), but not a dealbreaker.

CMakeLists.txt Outdated Show resolved Hide resolved
@fodinabor fodinabor force-pushed the feature/rocm-clang-version-check branch from f760932 to 7b77785 Compare August 9, 2022 06:15
@fodinabor fodinabor force-pushed the feature/rocm-clang-version-check branch from 7b77785 to 59a0fb7 Compare August 9, 2022 06:17
Copy link
Collaborator

@illuhad illuhad left a comment

Choose a reason for hiding this comment

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

Awesome!

@illuhad illuhad merged commit feec38e into AdaptiveCpp:develop Aug 9, 2022
@fodinabor fodinabor deleted the feature/rocm-clang-version-check branch August 9, 2022 12:34
@illuhad
Copy link
Collaborator

illuhad commented Aug 10, 2022

FWIW, this does not seem to work on Arch Linux with the rocm-arch AUR packages (https://github.com/rocm-arch/rocm-arch/blob/master/rocm-llvm/PKGBUILD):

$ /opt/rocm/llvm/bin/clang++ --version
clang version 14.0.0 (https://aur.archlinux.org/rocm-llvm.git bc7001d688249409ad3ba05561595e7bec515bdf)

Not a priority since Arch Linux is not an official supported ROCm target, but worth pointing out.

@fodinabor
Copy link
Collaborator Author

Ah, I've been using https://aur.archlinux.org/packages/rocm-llvm on Manjaro, which apparently poses as the "official" AMD clang.
Might be worth considering one of the alternative approaches:

  • Use a two-step process: figuring out that we have a ROCm clang (path starts with either /opt/rocm/ or ${ROCM_PATH}, or check whether clang++ --version contains either rocm, roc-(version) or AMD clang) and then use $ROCM_PATH/llvm/bin/clang -v -x hip < /dev/null |& grep HIP to get the version.
  • Rely on the HIP CMake integration

I don't know much about when the CMake integration was added / how reliably it can be used.
The first approach I'd expect to also theoretically support some older ROCm versions.
Is that something worth pursuing?

@illuhad
Copy link
Collaborator

illuhad commented Aug 10, 2022

hu, I think this is the package from the ROCm-Arch project and the same one that I am using. Do you get a different output there?

I think getting ROCm version from clang -v -x hip is probably a good idea since it offloads version detection to clang. However in practice we might have to give it additional arguments such as --rocm-path if ROCm lives in a non-standard location.

Another idea to check for ROCm-clang might be to check whether clang lives in a subdirectory of $ROCM_PATH. It's not bulletproof, but since the other ideas seem to be heuristics as well, this is maybe one that might work well in practice?

@fodinabor
Copy link
Collaborator Author

I do get

$ /opt/rocm/llvm/bin/clang++ --version
AMD clang version 14.0.0 (https://github.com/RadeonOpenCompute/llvm-project roc-5.2.0 22204 50d6d5d5b608d2abd6af44314abc6ad20036af3b)

For a refined version using the 2-stage approach see #807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake options to select the ROCm LLVM
3 participants