ginkgo icon indicating copy to clipboard operation
ginkgo copied to clipboard

Modernize CMake setup for HIP

Open upsj opened this issue 1 year ago • 5 comments

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

  • CUDA device architectures can be auto-detected since CMake 3.18, so if we push the requirements for the CUDA backend past that, we no longer need to maintain CAS.
  • 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.
  • CMAKE_<LANG>_FLAGS is the default way of adding flags to the entire library, I think we should use it instead of our custom flags.

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 avatar May 07 '23 11:05 upsj

Do we push to have this in 1.6.0 ?

pratikvn avatar May 08 '23 06:05 pratikvn

@pratikvn No, I would aim for the next release

upsj avatar May 08 '23 06:05 upsj

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.

yhmtsai avatar Jun 13 '23 07:06 yhmtsai

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_<LANG>_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 avatar Jun 13 '23 09:06 upsj

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

upsj avatar Jul 31 '23 21:07 upsj

@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).

upsj avatar Apr 12 '24 14:04 upsj

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

lahwaacz avatar Apr 12 '24 15:04 lahwaacz