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

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:921
   HelpText<"HIP runtime installation path, used for finding HIP version and adding HIP include path.">;
+def amdgpu_arch_tool_path_EQ : Joined<["--"], "amdgpu-arch-tool-path=">, Group<i_Group>,
+  HelpText<"Path to amdgpu_arch tool, used for detecting AMD GPU arch in the system.">;
----------------
I'd expect path to be the directory in which the tool is found, whereas this is used to name the tool itself. Perhaps 'amdgpu_arch_tool='? We might be able to write the default string inline as part of the argument definition, otherwise the current handling looks ok


================
Comment at: clang/test/Driver/amdgpu-openmp-system-arch.c:1
+// REQUIRES: system-linux
+// REQUIRES: x86-registered-target
----------------
This seems fairly clear. We might want to drop the fcuda-is-device from the regex as it doesn't matter for the test.

Failing cases (maybe driven by a separate C file) are probably:
print nothing, return 0
print nothing, return 1
print two different strings, return 0



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99949