[PATCH] D153725: [clang] Make amdgpu-arch tool work on Windows

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/tools/amdgpu-arch/AMDGPUArch.cpp:47
 
-  // Attempt to load the HSA runtime.
-  if (llvm::Error Err = loadHSA()) {
-    logAllUnhandledErrors(std::move(Err), llvm::errs());
-    return 1;
-  }
-
-  hsa_status_t Status = hsa_init();
-  if (Status != HSA_STATUS_SUCCESS) {
-    return 1;
-  }
-
-  std::vector<std::string> GPUs;
-  Status = hsa_iterate_agents(iterateAgentsCallback, &GPUs);
-  if (Status != HSA_STATUS_SUCCESS) {
-    return 1;
-  }
-
-  for (const auto &GPU : GPUs)
-    printf("%s\n", GPU.c_str());
-
-  if (GPUs.size() < 1)
-    return 1;
-
-  hsa_shut_down();
-  return 0;
+#ifdef _WIN32
+  return printGPUsByHIP();
----------------
jhuber6 wrote:
> yaxunl wrote:
> > jhuber6 wrote:
> > > Doesn't LLVM know if it's being built for Windows? Maybe we should key off of that instead and then conditionally `add_sources` for a single function that satisfies the same "print all the architectures" thing.
> > When this code is compiled on Windows, the compiler predefines `_WIN32`, so it should work.
> > 
> > I tried to tweak cmake files of amdgpu-arch to selectively add source files for Windows and non-windows but it did not work. If you have a file in that directory that is not included in any target, cmake will report an error. Seems there is a mechanism in CMake files for clang tools not allowing any 'dangling' source files.
> The proper way to do that is to add it to a new subdirectory and conditionally do `add_subdirectory`. Something like
> ```
> HSA/GetAMDGPUArch.cpp
> HIP/GetAMDGPUArch.cpp
> ```
> It's not a big deal, but I just feel like including unused symbols in the binary on Linux isn't ideal. Up to you if you want to put in the effort.
The HIP version actually works on both Linux and Windows. I am not sure whether one day we want to use it on Linux too since it supports target ID features.

Also, I kind of think it is overkill to have separate directories for Windows and Linux for this simple program.


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

https://reviews.llvm.org/D153725