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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to