yaxunl marked 3 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:
> 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.


================
Comment at: clang/tools/amdgpu-arch/AMDGPUArchByHIP.cpp:80
+    if (err != hipSuccess) {
+      llvm::errs() << "Failed to get device id for ordinal " << i << "\n";
+      return 1;
----------------
arsenm wrote:
> single quotes around '\n'
will do


================
Comment at: clang/tools/amdgpu-arch/AMDGPUArchByHIP.cpp:91
+    }
+    printf("%s\n", prop.gcnArchName);
+  }
----------------
arsenm wrote:
> llvm::outs
will do


================
Comment at: clang/tools/amdgpu-arch/AMDGPUArchByHSA.cpp:114
+  for (const auto &GPU : GPUs)
+    printf("%s\n", GPU.c_str());
+
----------------
arsenm wrote:
> llvm::outs()?
will do


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

Reply via email to