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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
