tra 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) {
----------------
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.
================
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();
----------------
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.
================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:27-28
+// only one matching sub-directory found, append the sub-directory to Path. If
+// there is no matching sub-directory or there are more than one matching
+// sub-directories, diagnose them. Returns true if there is only one matching
+// sub-directory.
----------------
yaxunl wrote:
> tra wrote:
> > How are users expected to handle cases when they do have multiple rocm
> > versions installed with spack (I assume that having multiple packages *is*
> > possible. E.g. package-4.0, package-3.7 should be able to co-exist). They
> > may have legitimate reasons to have more than one rocm version installed
> > *and* they may need to be able to build with a particular one.
> >
> > Can users use `--hip-path=specific/hip/version/in-spack` ?
> Yes. --hip-path is introduced for that purpose.
So, `--hip-path` is expected to point to the HIP runtime install directory and
could presumably be used to point anywhere, including an arbitrary spack
package. OK, this part works.
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
