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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 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();
----------------
arsenm wrote:
> 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
This should be what we do in the OpenMP runtime, should be able to add that feature.
```
  uint32_t name_len;                                                                                                                                                                      
  err = hsa_isa_get_info_alt(isa, HSA_ISA_INFO_NAME_LENGTH, &name_len);                                                                                                                        
  if (err != HSA_STATUS_SUCCESS) {                                                                                                                                                                                                   
    DP("Error getting ISA info length\n");                                                                                                                                                                                           
    return err;                                                                                                                                                                                                                      
  }                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                     
  char TargetID[name_len];                                                                                                                                                                                                           
  err = hsa_isa_get_info_alt(isa, HSA_ISA_INFO_NAME, TargetID);                                                                                                                                                                      
  if (err != HSA_STATUS_SUCCESS) {                                                                                                                                                                                                   
    DP("Error getting ISA info name\n");                                                                                                                                                                                             
    return err;                                                                                                                                                                                                                      
  } 
```

It's unfortunate that HSA does not work on Windows so we need to split this stuff up.


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

https://reviews.llvm.org/D153725