[PATCH] D97340: [HIP] Support Spack packages

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 6 inline comments as done.
yaxunl 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">;
----------------
tra wrote:
> Perhaps the existing `print-search-dirs` option would do the job?
The concern is similar to https://reviews.llvm.org/D79210:

There may be tools parsing print-search-dirs output, changing it may break those tools.



================
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();
----------------
tra wrote:
> 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;
> ```
> 
> 
We check whether ROCmSearchDirs is populated on line 166 and only populate it if not.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:207
+    // installation candidate for SPACK.
+    if (ParentName.startswith("llvm-amdgpu-")) {
+      auto SPACKReleaseStr =
----------------
tra wrote:
> 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.
> 
llvm-amdgpu Spack installation does not add extra files. It uses normal llvm cmake files to build and differs from a non-Spack llvm build only by name. However, its name follows a very unusual pattern llvm-amdgpu-<release>-<hash>, where <hash> is a string of 32 alphanumerical characters. I think we could detect it by checking the length of <hash>.


================
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:
> 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.
The situation is somehow similar to https://reviews.llvm.org/D79210

Another reason not to print this with -v is that normally users may not need this information unless clang cannot find ROCm and users want to know why. 


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


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

https://reviews.llvm.org/D97340