[PATCH] D97340: [HIP] Support Spack packages

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:163
 
 // For candidate specified by --rocm-path we do not do strict check.
+const SmallVectorImpl<RocmInstallationDetector::Candidate> &
----------------
tra wrote:
> I'm not quite sure which part of the code is related to the `do not do strict check`.  Perhaps move the comment closer to the code it describes and add the details what we'd do otherwise, so one can understand what's missing.
> 
> Also, adding a comment describing what the function does (returns candidate list, populating it once, if necessary) would be great.
will do


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:361-372
       auto Path = CandidatePath;
+      // Device library built by SPACK is installed to
+      // <rocm_root>/rocm-device-libs-<rocm_release_string>-<hash> directory.
+      if (Candidate.isSPACK()) {
+        auto SPACKPath = findSPACKPackage(D, Path, "rocm-device-libs",
+                                          Candidate.SPACKReleaseStr);
+        if (!SPACKPath.empty())
----------------
tra wrote:
> This could be simplified to
> ```
>  auto MaybeSpackPath = Candidate.isSPACK() ? findSPACKPackage() : "";
>  auto Path = MaybeSpackPath.empty() ? CandidatePath : MaybeSpackPath;
> 
> ```
> 
> We could further simplify it if we move  the `isSpack()` check into findSPACKPackage and make it return an empty string if it's not. 
will move isSPACK() inside findSPACKPackage 


================
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:
> yaxunl wrote:
> > 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.
> Agreed, the code works. That said, the fact that I didn't notice that is a point in case -- it's not obvious.
> It's much easier to keep track of what's going on when one function does one things without changing its behavior from one call to another.
> I'd at least add a comment around line 166 saying that just return the list if we've already populated it.
> 
will do


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:207
+    // installation candidate for SPACK.
+    if (ParentName.startswith("llvm-amdgpu-")) {
+      auto SPACKReleaseStr =
----------------
tra wrote:
> yaxunl wrote:
> > 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>.
> I think we're getting beyond the point of diminishing returns here. If we don't have a way to identify an SPACK package directly, detecting it by the name will never be 100% reliable. The current code will be correct most of the time. For those few cases where it may get it wrong, youser should be able to specify the correct path directly. No need to complicate it further -- it's not going to buy us much.
sure. will not check hash length.


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

https://reviews.llvm.org/D97340