[PATCH] D97340: [HIP] Support Spack packages

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl 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) {
----------------
tra wrote:
> `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`.
done


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


================
Comment at: clang/lib/Driver/ToolChains/ROCm.h:56
+    // convention <package_name>-<rocm_release_string>-<commit_hash>.
+    llvm::SmallString<0> SPACKReleaseStr;
+  };
----------------
tra wrote:
> 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. 
thanks. will use std::string


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

https://reviews.llvm.org/D97340