[PATCH] D97340: [HIP] Support Spack packages

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:25-31
+// Look for sub-directory starts with Prefix under Path. If there is one and
+// 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.
+static void handleSPACKPackage(const Driver &D, SmallString<0> &Path,
+                               StringRef Prefix) {
----------------
`void` function can not return `true`, so the comment needs updating.

Also, Using Path as both an input and an output is odd. We should probably return the full path or an empty string and call the function `findSPACKPackage`.


================
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.
----------------
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` ?


================
Comment at: clang/lib/Driver/ToolChains/ROCm.h:56
+    // convention <package_name>-<rocm_release_string>-<commit_hash>.
+    llvm::SmallString<0> SPACKReleaseStr;
+  };
----------------
Nit: using `SmallString` here and everywhere else in this patch does not buy us anything. `std::string` would be fine.
My own rule of thumb is to use standard types by default and optimized types when they are needed. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97340/new/

https://reviews.llvm.org/D97340