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