pdhaliwal added a comment.
On the testing perspective, the tool
================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:208
assert(GPUArch.startswith("gfx") && "Unsupported sub arch");
+ assert(!GPUArch.empty() && "Unable to detect system GPU");
----------------
JonChesterfield wrote:
> We shouldn't be handling unknown or missing march= fields with asserts. I see
> that this is already the case in multiple places, so let's go with a matching
> assert for this and aspire to fix that in a separate patch.
Matched this one with below.
================
Comment at: clang/tools/amdgpu-arch/AMDGPUArch.cpp:34
+ char GPUName[64];
+ Status = hsa_agent_get_info(Agent, HSA_AGENT_INFO_NAME, GPUName);
+ if (Status != HSA_STATUS_SUCCESS) {
----------------
JonChesterfield wrote:
> Does this null terminate for any length of GPU name? Wondering if we should
> explicitly zero out the last char.
Checked the rocr-runtime, the output is null terminated.
================
Comment at: clang/tools/amdgpu-arch/AMDGPUArch.cpp:45
+ if (Status != HSA_STATUS_SUCCESS) {
+ fprintf(stderr, "Unable to initialize HSA\n");
+ }
----------------
JonChesterfield wrote:
> Unsure these should be writing to stderr. We capture stdout, stderr probably
> goes to the user. We could exit 1 instead as clang is going to treat any
> failure to guess the arch identically
Remove fprintf
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99949/new/
https://reviews.llvm.org/D99949
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
