[PATCH] D97340: [HIP] Support Spack packages

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl 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) {
----------------
tra wrote:
> 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.
Will use PackageName and RocmVersion as arguments.

However, we use llvm::sys::path to manipulate paths and llvm::sys::path is using llvm::SmallString. If we return std::string or use StringRef for path argument, we have to convert them back and force.




================
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();
 
----------------
tra wrote:
> 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.
> 
> 
Will make SPACKReleaseStr a member of Candidate and allow mixed Spack candidates and non-Spack candidates. If clang path contains llvm-amdgpu but is not a Spack package, clang will still look for non-Spack ROCm candidates.

Also added parent of real path of clang as ROCm candidate. If clang is invoked through a sym link which is not under the real ROCm directory, clang will still be able to detect it. Also added --print-rocm-search-dirs for testing this.

When --hip-path/--rocm-path is set, the specified ROCm/HIP path is used. Spack detection is disabled, since the candidate specified by --rocm-path or --hip-path has empty SPACKReleaseStr.


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

https://reviews.llvm.org/D97340