haampie added a comment.
Herald added a subscriber: ormris.

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 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


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

Reply via email to