Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modernize CMake setup for HIP #1334

Merged
merged 22 commits into from
Apr 12, 2024
Merged

Modernize CMake setup for HIP #1334

merged 22 commits into from
Apr 12, 2024

Conversation

upsj
Copy link
Member

@upsj upsj commented May 7, 2023 •
edited
Loading

CMake has (or has gained) many features that we have our custom workarounds for, which I think we should remove soon

  • HIP support is much cleaner when it is handled by CMake >= 3.21, no more manually messing around with compiler flags, and we gain device auto-detection.
  • HIP's component-specific include folders are deprecated, we should just use the one provided by default to files with the HIP language property.

Some of these changes are more opinionated than others, so I'm happy to discuss them in more detail.

TODO:

  • Clean up get_info.cmake files
  • Clean up compiler flag handling
  • Update containers with later CMake versions

@upsj upsj added 1:ST:WIP This PR is a work in progress. Not ready for review. 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. labels May 7, 2023
@upsj upsj requested a review from a team May 7, 2023 11:08
@upsj upsj self-assigned this May 7, 2023
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. reg:example This is related to the examples. reg:benchmarking This is related to benchmarking. type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats mod:hip This is related to the HIP module. labels May 7, 2023
@pratikvn
Copy link
Member

pratikvn commented May 8, 2023

Do we push to have this in 1.6.0 ?

@upsj
Copy link
Member Author

upsj commented May 8, 2023

@pratikvn No, I would aim for the next release

@yhmtsai
Copy link
Member

yhmtsai commented Jun 13, 2023

we gain device auto-detection on HIP

Is it for NVIDIA device? We keep the CAS also for HIP on NVIDIA devices.
HIP already detects the AMD GPU automatically.

does this pull request drop the hip support on NVIDIA devices?

for CMAKE_<LANG>_FLAGS, it's for the entire library and the dependencies, right?
ginkgo own flag is only for ginkgo and it does not affect the others.

@upsj
Copy link
Member Author

upsj commented Jun 13, 2023

HIP already detects the AMD GPU automatically.

that's only true if we compile on a node that has AMD GPUs, otherwise compilation fails. CMake has a standard way to specify the architecture flags that also provides CAS-like support for autodetection at configure time instead of compile-time.

does this pull request drop the hip support on NVIDIA devices?

yes, I think this is a step we should take because otherwise we will be carrying around a lot of complexity (build system and source code) we don't need because we already have a CUDA backend.

for CMAKE__FLAGS, it's for the entire library and the dependencies, right?

no, CMake's scoping rules (initialized with values from CACHE, overwritten locally per nested subdirectory) allow us to change the CMAKE_<LANG>_FLAGS only inside the ginkgo dir, so the whole variable wasn't really needed from the beginning.

@upsj upsj mentioned this pull request Jul 11, 2023
Merged
@MarcelKoch MarcelKoch mentioned this pull request Jul 24, 2023
Merged
@upsj
Copy link
Member Author

upsj commented Jul 31, 2023

Good news on the hip-cuda front: https://gitlab.kitware.com/cmake/cmake/-/issues/25143

@MarcelKoch MarcelKoch mentioned this pull request Aug 3, 2023
Merged
@upsj upsj mentioned this pull request Jan 18, 2024
Merged
@upsj upsj changed the title Modernize CMake setup for CUDA and HIP Modernize CMake setup for HIP Jan 18, 2024
@upsj upsj mentioned this pull request Jan 18, 2024
Merged
1 task
@greole greole mentioned this pull request Jan 19, 2024
Open
@upsj upsj changed the base branch from develop to cmake_flags_cleanup January 19, 2024 12:54
@upsj upsj force-pushed the cmake_flags_cleanup branch from c9c200b to 24a945a Compare February 5, 2024 12:45
upsj and others added 11 commits April 9, 2024 18:24
@upsj
fix exception_helpers includes
7419034
@upsj
Revert "remove HIP CUDA support from source"
8ac6658 
This reverts commit c8ea7f4.
@upsj @yhmtsai
fix INSTALL.md documentation
d972798 
Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com>
@upsj
improve usage of GINKGO_HIP_PLATFORM
2c80e27
@upsj
disable simultaneous cuda/hip-cuda compilation in CI
60e363a
@upsj
add CMake minimum requirement for HIP
d1fb989
@upsj
fix HIP definitions
a8c859e
@upsj
disable HIP in last job
4bfb545
@upsj @yhmtsai @MarcelKoch
review updates
270cf53 
- fix incorrect CMake version requirement
- remove explicit RPATH additions
- clean up configure log

Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com>
Co-authored-by: Marcel Koch <marcel.koch@kit.edu>
@upsj
set CUDA and HIP standard explicitly
382e1f0
@upsj
remove hipconfig detection
5a0a476
@upsj upsj force-pushed the cmake_modernize branch from 9d01079 to 5a0a476 Compare April 9, 2024 16:34
@upsj upsj added 1:ST:do-not-merge Please do not merge PR this yet. 1:ST:run-full-test and removed 1:ST:ready-to-merge This PR is ready to merge. labels Apr 9, 2024
@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:do-not-merge Please do not merge PR this yet. labels Apr 10, 2024
@upsj
Copy link
Member Author

upsj commented Apr 12, 2024 •
edited
Loading

@lahwaacz do you absolutely need to support static linking with pkg-config (see #1104)? We are running into some issues where the autogenerated .pc fails because it is missing some information that cannot be encoded for pkg-config (i.e. custom linker for files involving device code).

@lahwaacz
Copy link
Contributor

@upsj I don't need static linking anymore, feel free to break this 😆

@upsj upsj merged commit 20f2a7e into develop Apr 12, 2024
10 of 15 checks passed
@upsj upsj deleted the cmake_modernize branch April 12, 2024 15:58
lahwaacz
lahwaacz reviewed Apr 13, 2024
View reviewed changes
cmake/hip.cmake
## Setup all CMAKE variables to find HIP and its dependencies
set(GINKGO_HIP_MODULE_PATH "${HIP_PATH}/cmake")
list(APPEND CMAKE_MODULE_PATH "${GINKGO_HIP_MODULE_PATH}")
if (GINKGO_HIP_PLATFORM MATCHES "${HIP_PLATFORM_AMD_REGEX}")
if (GINKGO_HIP_PLATFORM_AND)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo: AND -> AMD

@upsj upsj mentioned this pull request May 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lahwaacz lahwaacz lahwaacz left review comments

@MarcelKoch MarcelKoch MarcelKoch approved these changes

@yhmtsai yhmtsai yhmtsai approved these changes

Assignees

@upsj upsj

Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:run-full-test mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:testing This is related to testing. type:matrix-format This is related to the Matrix formats type:preconditioner This is related to the preconditioners
Projects
None yet
Milestone
Ginkgo 1.8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@upsj @pratikvn @yhmtsai @lahwaacz @MarcelKoch @ginkgo-bot