[SYCL][E2E] Use ROCM_PATH from the system environment
#18468
Conversation
For ROCm installations that aren't just in `/opt/rocm`, `clang` uses `ROCM_PATH` at runtime to find the ROCm device libraries, and this is done even in regular SYCL compilation. So this is needed for lit tests in certain configurations.
|
|
||
| # Add ROCM_PATH from system environment, this is used by clang to find ROCm | ||
| # libraries in non-standard installation locations. | ||
| llvm_config.with_system_environment("ROCM_PATH") |
There was a problem hiding this comment.
Actually I'm seeing now we should already check that envvar to set the cmake var, so maybe the problem is just that we don't pass --rocm-path on linux?
There was a problem hiding this comment.
This is needed for every single test not just when %hip_options are passed.
There's two different things going on, the %hip_options are used specifically for interop tests that use HIP code alongside SYCL code, they're used to link the runtime HIP libraries and include the HIP runtime headers.
But then for all SYCL tests, when compiling for AMD, clang uses some bitcode libraries provided by ROCm so it also needs to find the ROCm installation.
This patch is fixing that second use case, we could potentially use --rocm-path instead but that would have to be added to the regular %{build} substitution not just %hip_options, which may be slightly cleaner, but using ROCM_PATH is a bit easier and more flexible.
There was a problem hiding this comment.
got it, thanks. i didnt know the rocm path was needed when compiling for amd when not using hip.
i would prefer we minimize environment var usage in tests so i would prefer we remove the passing of --rocm-path as part of hip_options on Windows and just always add it to the %{build} substitution on all OSs when compiling for AMD
@aelovikov-intel Thoughts?
There was a problem hiding this comment.
IIUC, we can use this and remove all --rocm-paths. User's env is supposed to have ROCM_PATH (we copy it from there, after all), so they won't need to add extra env variables when reproducing outside LIT.
There was a problem hiding this comment.
yeah it seems like ROCM_PATH must always exist and point to the rocm install, i thought it was optional, in that case im fine to use the envvar
There was a problem hiding this comment.
I did have a look at the CMake and on top of that the HIP CMake is annoying so it's not straightforward to get the rocm path from it, so this is actually a lot easier.
I don't have a setup with Windows + ROCm, I can remove the --rocm-path there, but it's untested, and I'm not sure if ROCM_PATH works the same on Windows. That's the only place that uses --rocm-path as far as I can tell.
There was a problem hiding this comment.
let's try removing windows, if it fails just add it back, i vaguely remember the rocm detection on windows in upstream clang being basically non-existent so we might need it there
This may be covered by ROCM_PATH
sarnex
left a comment
sarnex
left a comment
There was a problem hiding this comment.
patch lgtm and if CI fails on windows lgtm to add --rocm-path back without another review
This reverts commit 7b2ac01.
|
There was one test failure on Windows without |
|
Sure, thanks. It's Windows so it's probably something really weird |
|
Technically we need the runtime team to review this so ping @aelovikov-intel :) |
aelovikov-intel
left a comment
aelovikov-intel
left a comment
There was a problem hiding this comment.
Technically we need the runtime team to review this so ping @aelovikov-intel :)
Can't you just get a membership in it by now? ;)
|
ill ask |
3 participants
For ROCm installations that aren't just in
/opt/rocm,clangusesROCM_PATHat runtime to find the ROCm device libraries, and this is done even in regular SYCL compilation. So this is needed for lit tests in certain configurations.This was accidentally removed in #17692 it doesn't cause any issues in the CI because the CI has ROCm installed in the standard
/opt/rocm, but it causes issues on local setups.Also remove
--rocm-pathon Windows, this should be covered byROCM_PATH.