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