[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal added a comment. I have put up a patch D102067 which uses __has_include as a workaround for header not found issue. @davezarzycki can you check if this resolves the issue? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal added a comment. Greg was also interested in having pci ids table in amdgpu-arch. And, keeping this table inside the target/amdgpu directory sounds like a good idea. Overall, I agree with not having dependency on hsa as it has caused many issues. Repository: rG LLVM Github

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. There is a potential hazard here for parallel compilation and to a lesser extent testing. (I have just learned that) kfd imposes a process limit which is low enough that we may see hsa_init fail under multiple processes. @jdoerfert you talked me out of

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. Yes, Fedora 34 (x86-64). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal added a comment. I could not find anything in the cmake files which could point to the issue mentioned here. @davezarzycki, are you on fedora/redhat? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal added a comment. I am investigating the find_package issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 ___ cfe-commits mailing list

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. We may need more robust error handling around this. I'm seeing test failures when building clang, `clang-13: error: Cannot determine AMDGPU architecture: $HOME/llvm-build/llvm/./bin/amdgpu-arch: Execute failed: No such file or directory. Consider passing it

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. None of those include paths look like the result of find_package locating a hsa so I'd guess this some quirk of cmake. It seems it is setting hsa-runtime64_FOUND but not doing the include path setup to match. I don't know how to debug that. @pdhaliwal?

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. It's still failing but I can disable HSA on this machine for now. FYI -- FAILED: tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o /p/tllvm/bin/clang++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. Live again as of 15be0c41d2e5 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. In D99949#2710273 , @davezarzycki wrote: > I removed all HSA/ROCM via the package manager and everything is fine now. > Have you considered using __has_include() instead of CMake? It seems to be > supported by all

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. I removed all HSA/ROCM via the package manager and everything is fine now. Have you considered using __has_include() instead of CMake? It seems to be supported by all contemporary compilers, even MSVC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. When find_package can find rocr, but it sets up include paths that don't work with it, I think that constitutes a broken rocr install. At least, one that is difficult to use from cmake. The packages are meant to deal with that sort of thing which suggests the

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. In D99949#2709785 , @JonChesterfield wrote: > It's not obvious to me why any of those stages would get a different result > for the search for rocr. Do you do things with chroot/jails to ensure > isolation for some of

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. It's not obvious to me why any of those stages would get a different result for the search for rocr. Do you do things with chroot/jails to ensure isolation for some of them? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. In D99949#2709673 , @JonChesterfield wrote: > That's interesting. The various permutations I have on disk are also all > path/include/hsa/hsa.h. The existing in tree use of hsa.h is the amdgpu > plugin, which uses

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. That's interesting. The various permutations I have on disk are also all path/include/hsa/hsa.h. The existing in tree use of hsa.h is the amdgpu plugin, which uses `#include "hsa.h"` and `#include `, which seems unlikely to be correct. I'm going to patch this

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. And if it helps: $ sudo find / -xdev -name hsa.h /usr/include/hsa/hsa.h /opt/rocm-3.10.0/include/hsa/hsa.h /opt/rocm-4.0.0/include/hsa/hsa.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. In D99949#2709497 , @JonChesterfield wrote: > In D99949#2709297 , @davezarzycki > wrote: > >> This change broke multi-stage builds (even if AMDGPU is disabled as a >> target). The

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. In D99949#2709297 , @davezarzycki wrote: > This change broke multi-stage builds (even if AMDGPU is disabled as a > target). The problem is that clang/tools/amdgpu-arch/AMDGPUArch.cpp can't > find hsa.h. Is there a quick

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

davezarzycki added a comment. This change broke multi-stage builds (even if AMDGPU is disabled as a target). The problem is that clang/tools/amdgpu-arch/AMDGPUArch.cpp can't find hsa.h. Is there a quick fix? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG722d4d8e7585: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed (authored by pdhaliwal). Repository: rG LLVM Github Monorepo

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 339235. pdhaliwal added a comment. Replaced the return commands in test scripts with exit command. It seems like return is handled bit differently on fedora/rhel machines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. I was under the impression that `#!/usr/bin/env sh` is a sensible invocation for running a shell on various systems. The current theory for this struggling with the ppc buildbot is that fedora doesn't support that. Ad hoc searching suggests 'sh' is required to

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3194761d2763: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed (authored by pdhaliwal). Repository: rG LLVM Github Monorepo

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LG, thank you for the debugging, and for the more descriptive failure reporting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 338843. pdhaliwal marked 2 inline comments as done. pdhaliwal added a comment. Review comments addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 Files:

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. Couple of minor points above. I think the increase in error reporting granularity will be helpful if this falls over in the field, as well as helping if we need a third try to get through PPC CI Comment at:

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 338810. pdhaliwal added a comment. Reopening this. This version is supposed to fix the buildbot failures on PPC machines. Since I don't have PPC machine I am not sure if this will work. But the logic followed here is motivated from Clang ::

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. The failing log pointed at `"-mllvm" "--amdhsa-code-object-version=4"`, which is a hard error if the amdgpu triple is missing from the llvm. I see the test features amdgpu-registered-target. Perhaps that does not behave as one might wish? I'd suggest

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal added a comment. I've reverted this from main for now as there seems to be issue with executing test script on some CI machines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7029cffc4e78: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed (authored by pdhaliwal). Repository: rG LLVM Github Monorepo

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) t-tye wrote: > JonChesterfield wrote: > >

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

t-tye added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) JonChesterfield wrote: > JonChesterfield wrote: > >

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

gregrodgers accepted this revision. gregrodgers added a comment. This revision is now accepted and ready to land. I am removing my objection with the understanding that we will either replace or enhance amdgpu-arch with the cross-architecture tool offload-arch as described in my comments above.

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) JonChesterfield wrote: > gregrodgers wrote: > >

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) gregrodgers wrote: > What happens when

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

gregrodgers added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) What happens when /opt/rocm is not available?

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield accepted this revision. JonChesterfield added a comment. I've built this, checked it behaves as expected, checked clang does something reasonable when the executable is missing. All looks good to me, explicitly accepting. We may need an internal call with Greg to work out how

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 337640. pdhaliwal added a comment. Rebase and review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 Files:

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. In D99949#2689583 , @gregrodgers wrote: > Dependence on hsa is not necessary. The amdgpu and nvidia drivers both use > PCI codes available in /sys . We should use architecture independent methods > as much as possible.

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

gregrodgers added a comment. Dependence on hsa is not necessary. The amdgpu and nvidia drivers both use PCI codes available in /sys . We should use architecture independent methods as much as possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. In D99949#2688869 , @gregrodgers wrote: > 1. It does not provide the infrastructure to identify runtime capabilities to > satisfy requirements of a compiled image. I believe we only require a value for '-march=' to

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

gregrodgers requested changes to this revision. gregrodgers added a comment. This revision now requires changes to proceed. I have two serious concerns with this tool . 1. It does not provide the infrastructure to identify runtime capabilities to satisfy requirements of a compiled image. 2.

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. New tests look good to me, thanks. Comment at: clang/include/clang/Driver/Options.td:922 +def amdgpu_arch_tool_EQ : Joined<["--"], "amdgpu-arch-tool=">, Group, + HelpText<"Path to amdgpu_arch tool, used for detecting AMD GPU arch in the

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 336353. pdhaliwal marked an inline comment as done. pdhaliwal added a comment. Fix permissions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 Files:

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 336351. pdhaliwal added a comment. Added tests for the failing cases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 Files:

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/include/clang/Driver/Options.td:921 HelpText<"HIP runtime installation path, used for finding HIP version and adding HIP include path.">; +def amdgpu_arch_tool_path_EQ : Joined<["--"], "amdgpu-arch-tool-path=">,

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 336037. pdhaliwal added a comment. Herald added subscribers: jansvoboda11, dang. - Addressed review comments - Added LIT test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. We'll have slightly indirect testing once this is used to enable D99656 . There are two pieces that can be tested: 1/ The clang handling. That we can test with a minor change. Add a command line argument to clang that specifies

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

jdoerfert added a comment. no strong opinion rn, I would add tests though Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 ___ cfe-commits mailing list

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. I'm happy with this as-is. @jdoerfert is this close enough to what you expected when we discussed this offline? Adding it directly to AMDGPU.h was a good suggestion. Makes it easy for other amdgpu language drivers to pick it up. That it's a binary on disk also

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

yaxunl added a comment. LGTM about AMDGPU toolchain change. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 ___ cfe-commits mailing list

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal added a comment. On the testing perspective, the tool Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:208 assert(GPUArch.startswith("gfx") && "Unsupported sub arch"); + assert(!GPUArch.empty() && "Unable to detect system GPU");

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal updated this revision to Diff 335761. pdhaliwal marked 10 inline comments as done. pdhaliwal added a comment. Addressed review comments. RE test: Since the tool is contingent on the results of HSA API call, adding a test which would always PASS on all the systems with different AMD

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:105 + +llvm::StringRef getSystemGPUArch(const ToolChain ) { + // detect the AMDGPU installed in system This function is useful for AMDGPU toolchain and HIP toolchain. Can it

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. This change is partly motivated by wanting to check in runtime tests for openmp that execute on whatever hardware is available locally. It is functionally similar to an out of tree bash script called mygpu that contains manually curated tables of pci.ids and to

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal planned changes to this revision. pdhaliwal added a comment. Working on tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 ___ cfe-commits mailing

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

pdhaliwal created this revision. pdhaliwal added reviewers: JonChesterfield, ronlieb, jdoerfert, ABataev, gregrodgers. Herald added subscribers: kerbowa, guansong, t-tye, tpr, dstuttard, yaxunl, mgorny, nhaehnle, jvesely, kzhuravl. pdhaliwal requested review of this revision. Herald added