Modernize CMake setup for HIP #1334

upsj · 2023-05-07T11:08:00Z

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

pratikvn · 2023-05-08T06:49:57Z

Do we push to have this in 1.6.0 ?

upsj · 2023-05-08T06:50:32Z

@pratikvn No, I would aim for the next release

yhmtsai · 2023-06-13T07:40:06Z

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 · 2023-06-13T09:47:09Z

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 · 2023-07-31T21:36:21Z

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

This reverts commit c8ea7f4.

Co-authored-by: Yuhsiang M. Tsai <[email protected]>

- fix incorrect CMake version requirement - remove explicit RPATH additions - clean up configure log Co-authored-by: Yuhsiang M. Tsai <[email protected]> Co-authored-by: Marcel Koch <[email protected]>

The static builds involve changing the linker for HIP, which can't really be represented in pkg-config

upsj · 2024-04-12T14:42:07Z

@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 · 2024-04-12T15:07:45Z

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

lahwaacz · 2024-04-13T09:56:50Z

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)


There is a typo: AND -> AMD

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 requested a review from a team May 7, 2023 11:08

upsj self-assigned this May 7, 2023

upsj mentioned this pull request Jul 11, 2023

Modernize CUDA CMake setup #1368

Merged

MarcelKoch mentioned this pull request Jul 24, 2023

Check downstream compilers #1372

Merged

MarcelKoch mentioned this pull request Aug 3, 2023

Enable CTest Resources #1373

Merged

upsj mentioned this pull request Jan 18, 2024

Fixing External Include of Build Directory #1532

Merged

upsj changed the title ~~Modernize CMake setup for CUDA and HIP~~ Modernize CMake setup for HIP Jan 18, 2024

upsj mentioned this pull request Jan 18, 2024

Use native CMake variables for compiler flags #1535

Merged

1 task

greole mentioned this pull request Jan 19, 2024

installation errors related to does not support platform specification, but platform hpsim/OGL#103

Open

upsj changed the base branch from develop to cmake_flags_cleanup January 19, 2024 12:54

upsj force-pushed the cmake_modernize branch from 85e9c6a to d81aeac Compare January 22, 2024 09:06

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

fix exception_helpers includes

7419034

Revert "remove HIP CUDA support from source"

8ac6658

This reverts commit c8ea7f4.

fix INSTALL.md documentation

d972798

Co-authored-by: Yuhsiang M. Tsai <[email protected]>

improve usage of GINKGO_HIP_PLATFORM

2c80e27

disable simultaneous cuda/hip-cuda compilation in CI

60e363a

add CMake minimum requirement for HIP

d1fb989

fix HIP definitions

a8c859e

disable HIP in last job

4bfb545

review updates

270cf53

- fix incorrect CMake version requirement - remove explicit RPATH additions - clean up configure log Co-authored-by: Yuhsiang M. Tsai <[email protected]> Co-authored-by: Marcel Koch <[email protected]>

set CUDA and HIP standard explicitly

382e1f0

remove hipconfig detection

5a0a476

upsj force-pushed the cmake_modernize branch from 9d01079 to 5a0a476 Compare April 9, 2024 16:34

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

fix HIP static builds

6422245

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 added 5 commits April 10, 2024 19:17

move to ROCm 5.1.4

55417be

disable buggy hipFFT tests

18e8946

fix compile def propagation for pkg-config

d5153f4

create pkg-config file for shared libs only

5256381

The static builds involve changing the linker for HIP, which can't really be represented in pkg-config

make test_pkgconfig optional

2494442

disable pkg-config only for HIP

a8a407f

upsj merged commit 20f2a7e into develop Apr 12, 2024
10 of 15 checks passed

upsj deleted the cmake_modernize branch April 12, 2024 15:58

lahwaacz reviewed Apr 13, 2024

View reviewed changes

upsj mentioned this pull request May 17, 2024

ROCm / HIP PATHs related ginkgo installation problems on Ubuntu 22.04 #1614

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize CMake setup for HIP #1334

Modernize CMake setup for HIP #1334

upsj commented May 7, 2023 •

edited

Loading

pratikvn commented May 8, 2023

upsj commented May 8, 2023

yhmtsai commented Jun 13, 2023

upsj commented Jun 13, 2023

upsj commented Jul 31, 2023

upsj commented Apr 12, 2024 •

edited

Loading

lahwaacz commented Apr 12, 2024

lahwaacz Apr 13, 2024

Modernize CMake setup for HIP #1334

Modernize CMake setup for HIP #1334

Conversation

upsj commented May 7, 2023 • edited Loading

pratikvn commented May 8, 2023

upsj commented May 8, 2023

yhmtsai commented Jun 13, 2023

upsj commented Jun 13, 2023

upsj commented Jul 31, 2023

upsj commented Apr 12, 2024 • edited Loading

lahwaacz commented Apr 12, 2024

lahwaacz Apr 13, 2024

Choose a reason for hiding this comment

upsj commented May 7, 2023 •

edited

Loading

upsj commented Apr 12, 2024 •

edited

Loading