[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

[Pushpinder Singh via Phabricator via cfe-commits]

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