electronic-structure / SIRIUS

Domain specific library for electronic structure calculations
BSD 3-Clause "New" or "Revised" License
127 stars 40 forks source link

rely on `CMAKE_CUDA_ARCHITECTURES` / enable_language(HIP) #819

Closed simonpintarelli closed 1 year ago

simonpintarelli commented 1 year ago
toxa81 commented 1 year ago

How CMake treats ROCM architecture?

toxa81 commented 1 year ago

Regarding spack: we will soon have to intoruduce changes related to adding Wannier90 interface. We can combine the changes in one spack PR.

simonpintarelli commented 1 year ago

How CMake treats ROCM architecture?

It supports the same way as for cuda since v3.21: https://cmake.org/cmake/help/v3.21/variable/CMAKE_HIP_ARCHITECTURES.html?highlight=hip We can update the minimum requirement and use it

toxa81 commented 1 year ago

@mtaillefumier what is the minimum cmake version for cp2k? If v3.21 brings HIP support, can we switch to it?

mtaillefumier commented 1 year ago

Please do cmake v3.22 actually minimum requirement for cp2k but 3.21 should work as well

toxa81 commented 1 year ago

@AdhocMan If we switch to cmake-3.21, can this part be changed in favour of CMAKE_HIP_ARCHITECTURES?

  # FindHIP module provides compilation command for GPU code
  if(NOT HIP_HCC_FLAGS)
    message(STATUS "Using default AMD gpu targets: gfx803, gfx900, gfx906. Set HIP_HCC_FLAGS to override.")
    set(HIP_HCC_FLAGS ${HIP_HCC_FLAGS} --amdgpu-target=gfx803 --amdgpu-target=gfx900 --amdgpu-target=gfx906)
  endif()
AdhocMan commented 1 year ago

@AdhocMan If we switch to cmake-3.21, can this part be changed in favour of CMAKE_HIP_ARCHITECTURES?

  # FindHIP module provides compilation command for GPU code
  if(NOT HIP_HCC_FLAGS)
    message(STATUS "Using default AMD gpu targets: gfx803, gfx900, gfx906. Set HIP_HCC_FLAGS to override.")
    set(HIP_HCC_FLAGS ${HIP_HCC_FLAGS} --amdgpu-target=gfx803 --amdgpu-target=gfx900 --amdgpu-target=gfx906)
  endif()

Yes, this part can be removed. Also, when creating the library, you will no longer need to use the HIP_ADD_LIBRARY macro and you can delete all the associated files. The only important thing to remember is to set the -fno-gpu-rdc hip flag, such that you don't need to use the hip compiler for linking (there is no CMAKE_HIP_SEPARABLE_COMPILATION for some reason). And of course to set CMAKE_HIP_STANDARD (similar to CMAKE_CUDA_STANDARD) equal to CMAKE_CXX_STANDARD.

toxa81 commented 1 year ago

@mtaillefumier and @AdhocMan can I please ask you to polish this PR for hip-related stuff? Then we can switch to cmake>=3.21

mtaillefumier commented 1 year ago

no problems we can take care of this.

simonpintarelli commented 1 year ago

I've added enable_language(HIP) and set_source_files_properties(${_CUSOURCES} PROPERTIES LANGUAGE HIP LINKER_LANGUAGE HIP), but not -fno-gpu-rdc. I copied this from DLA-F. Not sure this is the correct way of doing it. I couldn't find the relevant information in cmake documentation. It compiles and links with rocm 5.2.3.

mtaillefumier commented 1 year ago

that's correct. Curiously I did exactly the same than you did and I can not get it to work properly. enable_language(HIP) is particularly unstable apparently.

mtaillefumier commented 1 year ago

I have others changes I wish to include as well but it will have to wait for next week.

simonpintarelli commented 1 year ago

LGTM