[PATCH] D97340: [HIP] Support Spack packages

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:3535-3536
   HelpText<"Print the registered targets">;
+def print_rocm_search_dirs : Flag<["-", "--"], "print-rocm-search-dirs">,
+  HelpText<"Print the paths used for finding ROCm installation">;
 def private__bundle : Flag<["-"], "private_bundle">;
----------------
Perhaps the existing `print-search-dirs` option would do the job?


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:180
   if (!RocmPathArg.empty()) {
-    Candidates.emplace_back(RocmPathArg.str());
-    return Candidates;
+    ROCmSearchDirs.emplace_back(RocmPathArg.str());
+    DoPrintROCmSearchDirs();
----------------
We call getInstallationPathCandidates() more more than one place, so we may end up with the same path added multiple times.

I think population of the candidate list should be separated from reading it back, and should be done only once.
E.g.
```
  if (!isCandidateListReady) 
     initCandidateList(); // populates ROCmSearchDirs, sets isCandidateListReady
  return ROCmSearchDirs;
```




================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:207
+    // installation candidate for SPACK.
+    if (ParentName.startswith("llvm-amdgpu-")) {
+      auto SPACKReleaseStr =
----------------
What would be a consequence of mistakingly detecting a non-SPACK installation as SPACK?

I'm still not quite comfortable with using a path name, which can be arbitrarily named by a user, as a way to detect particular installation method.
Do SPACK packages have something *inside* the package directory that we could use to tell that it is indeed an SPACK package? Some sort of metadata or version file, perhaps?

It's probably OK to rely on the path naming scheme, but there are still corner cases it can't handle.
I could have a setup when the path-name based detection would still fail despite considering both the reaolved and unresolved symlink names.
E.g. 
```
/pkg/llvm-amdgpu-hash -> /pkg/some-other-real-dir-name
/usr/local/bin/clang -> /pkg/llvm-amdgpu-hash/bin/clang
```
If I compile with `/usr/local/bin/clang` neither the realpath nor the unresolved symlink will have 'llvm-amdgpu'. 

Granted, it's a somewhat contrived setup. Using the name as the hint would work for the standard setups, so we can live with that, provided that we can always tell compiler where to find the files it needs, if the user has an unusual install.



================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:32
+static llvm::SmallString<0> findSPACKPackage(const Driver &D,
+                                             const llvm::SmallString<0> &Path,
+                                             StringRef Prefix) {
----------------
yaxunl wrote:
> 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.
> 
> 
It should still work. The use cases below look like this:
```
        auto SPACKPath = findSPACKPackage(D, Path, "rocm-device-libs",
                                          Candidate.SPACKReleaseStr);
        if (!SPACKPath.empty())
          Path = SPACKPath;
      }
      for (auto SubDir : SubDirs)
        llvm::sys::path::append(Path, SubDir);
```

It's the `Pack` that needs to be a `SmallString`. Assigning an `std::string` to it should work  -- it has `operator=(StringRef)` and `StringRef` can be created from a `std::string`.

I'll leave it up to you. If `std::string` is inconvenient to use, the `SmallString` is fine.



================
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();
 
----------------
yaxunl wrote:
> 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.
> Also added --print-rocm-search-dirs for testing this.

Maybe we could just enable that with `-v`.  We already are printing various locations for GCC installations, etc.


================
Comment at: clang/lib/Driver/ToolChains/ROCm.h:48
+    // convention <package_name>-<rocm_release_string>-<hash>.
+    std::string SPACKReleaseStr;
+
----------------
I'd add a `bool isSPACK()` method to consolidate the SPACK/non-SPACK determination into a single location.


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

https://reviews.llvm.org/D97340