yaxunl marked 3 inline comments as done.
yaxunl added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:32
+static llvm::SmallString<0> findSPACKPackage(const Driver &D,
+ const llvm::SmallString<0> &Path,
+ StringRef Prefix) {
----------------
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.
================
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:
> 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.
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
