[clang][amdgpu] Add LLVM_ENABLE_HSA flag or similar

[aaronmondal]

Recent patches have updated the amdgpu-arch tool.

With the current behavior, if a local HSA is found, the tool will be built with it.

HSA is part of the rocr-runtime which depends on the rocm-device-libs and those need clang to build. If an older HSA is on the system the tool may fail to build with a newer Clang, i.e. Clang will fail to build and the error will only be noticeable during linking of the tool. Old clang won't be able to build new HSA either.

Deadlock, until the old HSA is forcefully removed :(

I think there needs to be an LLVM_ENABLE_HSA flag or similar and that flag needs to be disabled by default. Otherwise we may get deadlocks on source based distros where users need to jump through hoops to get things running again.

cc @jhuber6 @JonChesterfield @arsenm @littlewu2508 @mgorny

[aaronmondal]

This is part of the 16 branch, potentially breaking user's distros if this makes it into the stable release without further changes.

Issue originally found in ROCm/ROCm#1880.

[littlewu2508]

This situation is also similar to openmp amdgpu plugin, firstly reported at https://bugs.gentoo.org/835095

[littlewu2508]

I think there needs to be an LLVM_ENABLE_HSA flag or similar and that flag needs to be disabled by default.

Actually when not finding HSA on system, amdgpu-arch and openmp amdgpu plugin turn to dlopen implementation, so the possible solution would be choosing the dlopen way by default. But maybe there are better approaches.

[jhuber6]

I think there needs to be an LLVM_ENABLE_HSA flag or similar and that flag needs to be disabled by default.

Actually when not finding HSA on system, amdgpu-arch and openmp amdgpu plugin turn to dlopen implementation, so the possible solution would be choosing the dlopen way by default. But maybe there are better approaches.

Yes, this is the intention. Can you give an example of the linker error? If the user has an HSA in their path it should presumably work. When was the switch to make the ROCR runtime runtime depend on the device libraries? I remember hearing about it at some point and figured it would lead to nightmarish dependency chains and make ROCm even more impossible to build.

[aaronmondal]

Actually when not finding HSA on system, amdgpu-arch and openmp amdgpu plugin turn to dlopen implementation, so the possible solution would be choosing the dlopen way by default. But maybe there are better approaches.

For dependencies like zlib there is an LLVM_ENABLE_ZLIB flag that globally configures zlib support in all llvm subprojects. Since everyone and their grandparents have zlib on their system and since zlib hasn't really changed for like 100 years, defaulting that to "on" makes sense.

Significantly less users will have HSA though. So I agree that defaulting to dlopen makes much more sense 👍 It would also (hopefully?) not be a change too hard to backport. Having a LLVM_ENABLE_HSA configure all HSA dependencies seems like a good idea to me, but I may be overlooking something.

Yes, this is the intention. Can you give an example of the linker error? If the user has an HSA in their path it should presumably work. When was the switch to make the ROCR runtime runtime depend on the device libraries? I remember hearing about it at some point and figured it would lead to nightmarish dependency chains and make ROCm even more impossible to build.

This and the comments below: ROCm/ROCm#1880 (comment)

IMO the autodetection approach is one of the major reasons that the ROCm stack is so difficult to build in the first place. Everything assumes that all autodetected dependencies are API/ABI compatible which is not the case. Instead of a flag it may be possible to backport df04883, but again this would only mask the issue.

[littlewu2508]

When was the switch to make the ROCR runtime runtime depend on the device libraries? I remember hearing about it at some point and figured it would lead to nightmarish dependency chains and make ROCm even more impossible to build.

Are you worried about llvm/clang -> ROCR runtime -> device libs -> llvm/clang circular dependency?

There are two solutions:

1. This issue, use dlopen implementation, turn build time dependency to runtime dep only.
2. Disable ROCR runtime's image extension.

[aaronmondal]

Distros like Gentoo with the clang profile use Clang/LLVM as their host compiler. When clang is the host compiler and not just a guest compiler where you can "delete clang", these breakages can brick an entire system.

Users may have completely different versions of HSA on their system (some installed via a package managers, some installed manually). So I think its incredibly risky to autodetect this by default unless there was "perfect" backwards compatibility for all HSA versions. Perfect backwards compatibility seems unrealistic.

[littlewu2508]

IMO the autodetection approach is one of the major reasons that the ROCm stack is so difficult to build in the first place. Everything assumes that all autodetected dependencies are API/ABI compatible which is not the case. Instead of a flag it may be possible to backport df04883, but again this would only mask the issue.

I'd like to believe the difficulties mostly comes from ROCm's development cycle is asynchronized with llvm, the former is still based on their own llvm-project tree because they want new features land in their releases ASAP (That's not bad! Catch up cuda :-). Things are getting better now, it's just half year ago Gentoo switched the ROCm stack for using upstream llvm-14 without hack on llvm side. New features are quickly pushed to ROCm's own llvm-project, and later get reviewed and enters upstream llvm main branch, and if it is unfortunate it has to wait half a year for another llvm release cycle, but ROCm has a faster pace -- it need to ship a new release including the newest feature. Take df04883 as example, I guess it will go to the llvm-17 release in normal situation, but since now ROCm master ROCm/ROCm-Device-Libs@8dc779e depend on that, which means ROCm releases in half a year you cannot get the newest ROCm release and llvm release compatible.

[jhuber6]

So, I'm wondering what the best solution is here. Because the HSA library is an optional dependency I assumed that if we could find an HSA on the system it would be sufficient to link against (Above version 1.2). The amdgpu-arch tool only uses four functions which aren't likely to change function in the future.

[aaronmondal]

I think it would be nice to put HSA behind a flag like every other external dependency. Things like zlib, zstd and libxml2 already work this way. They are also autodetected, but only if e.g. LLVM_ENABLE_HSA is set.

I'm not saying autodetection is bad in general. It just shouldn't be enabled by default. This may also allow simplifying the include logic in the amdgpu-arch tool (#ifdef LLVM_ENABLE_HSA etc).

[littlewu2508]

So, I'm wondering what the best solution is here. Because the HSA library is an optional dependency I assumed that if we could find an HSA on the system it would be sufficient to link against (Above version 1.2). The amdgpu-arch tool only uses four functions which aren't likely to change function in the future.

This is the exact behavior for upcoming llvm-16, but somehow it causes ROCm/ROCm#1880 (comment).

[jhuber6]

Is there an off chance that 067a5c6 solves this problem?