yaxunl added a comment. In D97340#2775477 <https://reviews.llvm.org/D97340#2775477>, @haampie wrote:
> Hi @tra and @yaxunl, I'm commenting as a reviewer of the spack pull request > for the rocm 4.2.0 ecosystem. First of all: thanks for caring about spack > installations, that's highly appreciated. > > However, this patch does not seem the right approach to me, for a couple > reasons: > > 1. LLVM should not be aware of spack -- it doesn't make sense. Spack doesn't > even have a stable 1.0 release, expect it to break and to be inconsistent > between versions. > 2. It's incorrect in multiple ways: (a) the directory structure name is > configurable in spack, not everybody has this <name>-<version>-<hash> > structure, so you shouldn't rely on it, (b) you are still not guaranteed to > pick up the correct llvm-amdgpu because it may be installed in a chained repo > on a shared HPC system, and it won't be in the same prefix folder at all (c) > spack's main selling point is it supports multiple installs of one package (a > hash also changes for the same llvm version if a downstream dep such as zlib > is updated), this patch just bails when it detect multiple installations > > Let's step back a bit. The problem you seem to be solving is the circular > dependency between llvm and rocm-device-libs which are separate packages in > spack; clang can't know where rocm-device-libs is at compile time because > rocm-device-libs depends on llvm. > > Clearly the solution is to build llvm together with rocm-device-libs, as > explained in the readme of > https://github.com/RadeonOpenCompute/ROCm-Device-Libs, we can fix that in > spack by adding a `resource` in the llvm package to pull in more sources, and > LLVM can happily live without knowing about spack. > > Thanks, > > Harmen Thanks for your feedback. Before this patch, clang assumes llvm, HIP and device lib directory follow ROCm directory structure. Since the option `-hip-path` and environment variable `HIP_PATH` have not been introduced to clang, there was no way for clang to specify or deduce HIP path if it does not follow ROCm directory structure. At that time, I thought the only way to support Spack package was to let clang know its naming convention and relative directory structure among llvm, HIP and device lib. That's what motivated this patch. The patch should not cause circular dependency on HIP or device library. There was a bug causing clang to fail if it did not find Spack HIP package or multiple Spack HIP packages. I apologize for the trouble caused by that. That bug was fixed by https://reviews.llvm.org/D102556. With that fix, clang will not fail if no Spack HIP package detected or multiple Spack HIP packages are detected. With that fix, Spack llvm-amdgpu package does not need to depend on Spack HIP and device-lib packages. llvm-amdgpu package can be built and installed first. Then device-lib package can be built with clang as before, since device-lib is written in OpenCL and the OpenCL code is compiled with -nogpulib, therefore clang should not try to detect device-lib when building device lib. Also clang does not need HIP to compile device-lib. Now that clang supports `-hip-path` option and `HIP_PATH` environment variable, it is possible to specify HIP location for Spack. The detection of Spack HIP and device-lib path become an optional feature for users' convenience. Without auto-detection of HIP and device-lib path, users have to use lengthy options `-hip-device-lib-path` and `-hip-path` to specify them, which is painful. That's why we want to detect them automatically, like how clang auto-detect gcc, msvc, CUDA SDK, and ROCm. Such auto-detection is optional. If clang cannot auto-detect HIP or device-lib, it just assumes they are not installed and will not fail. Users are always able to override it by using `-hip-device-lib-path` and `-hip-path`. Therefore for users it is not a loss to have auto-detection. Hopefully, by following proper heuristics based on naming and location conventions, we may be able to detect HIP and device-lib location automatically for common users, so that they can use clang out of box to compile HIP programs without specifying HIP and device-lib path explicitly. For cases where clang cannot auto-detect HIP and device-lib locations, users can always specify them with options or environment variables. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97340/new/ https://reviews.llvm.org/D97340 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
