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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
