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

[Matt Arsenault via Phabricator via cfe-commits]

arsenm 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();
----------------
yaxunl wrote:
> 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.
Why can't you get the target id features through the HSA path? I think there's value in going through the lowest level component to get the information


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

https://reviews.llvm.org/D153725