[PATCH] D97340: [HIP] Support Spack packages

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:32
+static llvm::SmallString<0> findSPACKPackage(const Driver &D,
+                                             const llvm::SmallString<0> &Path,
+                                             StringRef Prefix) {
----------------
Based on how the function is used, perhaps we should make it 
`static std::string findSPACKPackage(const Driver &D, StringRef Path, StringRef PackageName, StringRef RocmVersion)`  and fold the prefix construction (and possibly version non-emptiness check) into the function.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:354-372
+  CandidatesInfo CanInfo;
+  if (!HIPPathArg.empty())
+    CanInfo.Candidates.emplace_back(HIPPathArg.str(), /*StrictChecking=*/true);
+  else
+    CanInfo = getInstallationPathCandidates();
   auto &FS = D.getVFS();
 
----------------
So, normally `--hip-path` would point to the HIP package installation. 
However, is clang installation path contains `llvm-amdgpu` (and thus CanInfo.SPACKReleaseStr is set to whatever version may follow),
then `--hip-path` (and other candidate paths) directories will also be treated as an SPACK package directory and, if the `hip` package is found there, then *that* package directory will be used as the actual path to be used.

I see several potential issues with that.

- if clang path contains llvm-amdgpu, but it is *not* installed as an SPACK package We have no control over user's choice of the name for the installation path.
- what if clang is installed as an SPCAK, but is invoked via a symlink with the name that does not contain `llvm-amdgpu`
- the fact that `--hip-path`/`--rocm-path` behavior may change based on clang's executable path is rather peculiar. User-supplied parameters should not change their meaning depending on what you call the executable. 

I'd be OK to derive spack-based paths if user didn't provide the paths explicitly, but I don't think we should add any magic to explicit overrides.




================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:27-28
+// only one matching sub-directory found, append the sub-directory to Path. If
+// there is no matching sub-directory or there are more than one matching
+// sub-directories, diagnose them. Returns true if there is only one matching
+// sub-directory.
----------------
yaxunl wrote:
> tra wrote:
> > How are users expected to handle cases when they do have multiple rocm versions installed with spack (I assume that having multiple packages  *is* possible. E.g. package-4.0, package-3.7 should be able to co-exist). They may have legitimate reasons to have more than one rocm version installed *and* they may need to be able to build with a particular one.
> > 
> > Can users use `--hip-path=specific/hip/version/in-spack` ?
> Yes. --hip-path is introduced for that purpose.
So, `--hip-path` is expected to point to the HIP runtime install directory and could presumably be used to point anywhere, including an arbitrary spack package. OK, this part works.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97340/new/

https://reviews.llvm.org/D97340