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.

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

Build cp2k with cmake #2364

Merged
merged 13 commits into from Oct 27, 2022
Merged

Build cp2k with cmake #2364

merged 13 commits into from Oct 27, 2022

Conversation

mtaillefumier
Copy link
Contributor

@mtaillefumier mtaillefumier commented Oct 19, 2022

Note 1 : can we avoid running the full CI for the time being ?
Note 2: the two build systems will coexist for at least one or two versions.

This PR is long overdue but this work reaches the point where it become usable to build cp2k with cmake. I kept the building process as simple as possible so that other devs can look around and tinker with it a little bit.

This build system does only one task, configuring and compiling cp2k. The dependencies should be compiled separately as a first step.

I am also fully aware of that it will not necessarily work for everyone the first time that's why I am asking devs, experienced users and package managers to test it and report issues as much as they can. I will open an issue on the cp2k github repo to regroup all of them in one place.

so far all dependencies should be detected with fluctuating success. The mandatory dependencies are

  • DBCSR
  • BLAS / LAPACK / SCALAPACK implementation
  • openmp
  • c, c++, fortran compilers

libxc, libint, cosma, libxsmm, fftw are turned on by default. All the other dependencies are off
All options are prefixed with CP2K_. This rule is enforced for the build system because it can potentially interfere with others options and variables names defined by the dependencies cmake config files (BLAS in particular). The -D__OPTIONS are unchanged.

the build system supports CUDA and HIP (as much as HIP goes expect some issues)

how to compile cp2k :

  • 'MKL' with 'CUDA' (fftw, mpi, libxc, cosma, libint2, libxsmm should be installed somewhere)

cmake -DCMAKE_INSTALL_PREFIX=/myprefix -DCP2K_BLAS_VENDOR=MKL -DCP2K_SCALAPACK_VENDOR=MKL -DCP2K_USE_ACCEL=CUDA -DCP2K_WITH_GPU=V100 ..

  • MKL without CUDA : remove the -DCP2K_USE_ACCEL

  • MKL with HIP replace CUDA with HIP and set -DCP2K_WITH_GPU=Mi250 (of instance). Note that these options have default values

  • Cray environments

  • with libsci and cuda support : 'MPICC=cc MPICXX=CC MPIFORT=ftn cmake -DCMAKE_INSTALL_PREFIX=/myprefix -DCP2K_BLAS_VENDOR=SCI -DCP2K_SCALAPACK_VENDOR=SCI -DCP2K_USE_ACCEL=CUDA -DCP2K_WITH_GPU=V100 ..`

  • with MKL see the mkl section because it is exactly the same command line.

list of known potential issues

  • Configuring cp2k with ROCM 5.0.x is likely to fail. There is a bug in the rocm cmake infrastructure that is hard to go around
  • Compiling cp2k woth ROCM 5.2.x will work but DBCSR won't work properly because of a bug in the rocm jiting functionality. It should be fixed in ROCM 5.3.x (so use it if you can)
  • The current DBCSR build system is missing one important line that makes the cp2k build system fails during link time if DBCSR is compiled with libxsmm support. I opened a PR that fix this issue.
  • gfortran does not seem to have a default include directory but it is only an issue if cp2k is installed in /usr (on unix)
  • All dependencies should be compiled with fortran support (if they generate the .mod files). I will add a check for this peculiar point eventually.
  • CUDA : the Nvidia hpc sdk install the cuda math libraries in a different directory that cuda. This can be source of errors when cmake is searching for cublas or cufft.
  • The generic detection of blas and lapack only cover the most important implementations of blas and lapack. It might still fail though because naming convention for these libraries is fluctuating.
  • Cray libsci : dependencies might compile and link with libsci_mp.so vairants while cp2k will link with libsci.so by default. It will generate a warning at the end of cp2k execution. The solution is to pass -fopenmp during link time.
  • some of the FindPackage.cmake files are rudimentary.
  • Summit cluster. We need a custom command line to make it work. The only issue is really blas and lapack on this system.

missing things and improvements

  • Add comments in the cmake build scripts
  • test the cp2k installation itself (follows the default unix layout, data are set to /myprefix/share/cp2k/data)
  • Include the possibility to run the regtests with a simple make tests
  • Only include units files that need to be compiled when the option is on. It is partially done at that stage (we will have to delay this otherwise the current build system will break, I do not want that)
  • Fix the VERSION file
  • rename README.cmake to something more convenient (prettify fails)
  • review compilations options (maybe add some options for devs turn on/off some warnings)
  • add check for the module files. (for instance libint2.mod is missing on Fedora)

@oschuett
Copy link
Member

oschuett commented Oct 19, 2022

This is awesome! Adding a new build system to a grown code base is no easy feat. Great work!

Note 1 : can we avoid running the full CI for the time being ?

Yes, we can merge this PR without tests. Over the coming weeks we can then migrate one CI test at a time.

I noticed that many of the files have a BSD license from ETH. Could you reach out to the original authors and ask if they'd be ok with releasing these files directly under GPL?

@mtaillefumier
Copy link
Contributor Author

mtaillefumier commented Oct 20, 2022

This is awesome! Adding a new build system to a grown code base is no easy feat. Great work!

Note 1 : can we avoid running the full CI for the time being ?

Yes, we can merge this PR without tests. Over the coming weeks we can then migrate one CI test at a time.

I want to wait a little so that devs (mostly) try it and tell me what is failing. I will work on the hip side further (the compilation is not the issue, nor is cmake but rocm is).

Additional note : For internal reason, we are using spack + cp2k + cmake to actually build cp2k (with the default dependencies).

I noticed that many of the files have a BSD license from ETH. Could you reach out to the original authors and ask if they'd be ok with releasing these files directly under GPL?

I blindly copy pasted the license agreement we use almost everywhere but i do not think GPL is an issue at least not on my side.
Except FindMKL.cmake where I modified our internal version to better suit findBLAS.cmake all the others are my own writing. Tiziano should also agree to this request but we can switch to GPL in my point of view (FindMKL.cmake might still stay BSD I will ask the author).

@mtaillefumier mtaillefumier force-pushed the cmake_v6 branch 21 times, most recently from 378b9ba to 14cb4f9 Compare October 24, 2022 09:04
Copy link
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Very nice to see this! I've added a few comments that would make this a bit more idiomatic "modern" CMake, but none of the comments are blockers.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
set(CP2K_USE_HIP OFF)
set(CP2K_USE_CUDA OFF)

set(CMAKE_POSITION_INDEPENDENT_CODE ${CP2K_SET_POSITION_INDEPENDENT_CODE})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to having CP2K_SET_POSITION_INDEPENDENT_CODE instead of just setting CMAKE_POSITION_INDEPENDENT_CODE directly as a user?

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
message(STATUS " ")
message(STATUS "- COSMA")
message(STATUS " - include directories : " ${COSMA_INCLUDE_DIRS})
# message(STATUS " - libraries : " ${COSMA_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove or uncomment?

CMakeLists.txt Outdated
message(STATUS " - SPGLIB")
endif()

# if (NOT CP2K_USE_MPI) message(STATUS " - MPI") endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove or uncomment?

@@ -0,0 +1,123 @@
if(CMAKE_Fortran_COMPILER_ID STREQUAL "GNU")
set(CMAKE_Fortran_FLAGS
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all of these CMAKE_*_FLAGS definitions will override any user-defined CMAKE_*_FLAGS. I would recommend either appending to the cache variables or again using targets and target_compile_flags (together with generator expressions for the various conditionals).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the initial version for the time being but I will work on this to make simpler.

${DBM_SRCS_GPU})

set_target_properties(dbm_miniapp PROPERTIES LINKER_LANGUAGE C)
# target_compile_options(dbm_miniapp PUBLIC "${OpenMP_C_FLAGS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove or uncomment? I think the OpenMP::OpenMP_<lang> target should anyway be enough for this.

@msimberg
Copy link
Contributor

Summarizing my review comments:

  • Remove or uncomment commented out code
  • Change CMAKE_*_FLAGS to target_compile_flags
  • Consistent CMAKE_BUILD_TYPEs
  • Use option for boolean options
  • Use cmake_dependent_option for dependent options
  • systemd dependency for hostnamectl needed?
  • Use target_compile_features where possible (cxx_std_11)
  • Unnecessary enable_language (since set in project)?
  • Necessary to have separate POSITION_INDEPENDENT_CODE variable instead of using the standard CMake variable?
  • Fix typo in CP2K_SUPPORTED_ACCELEARTION_TARGETS

- use option(...) instead of set(...) when possible
- use cmake_dependent_option(...) instead of the if then else construct to set dependent options
- remove commented out code
- remove unnecessary enable_language
- fix typo in CP2K_SUPPORTED_ACCELERATION_TARGETS
- use target_compile_features(...) to set the different languages standards
- Remove the VERSON file. The version is set directly inside CMakeLists.txt or with a tag
- add cp2k.pc file
- add search of libint2.mod file
- rocm 5.3.0 works
@mtaillefumier
Copy link
Contributor Author

It is time to get this PR in the wild. Please quash all commits when the PR is merged as it is not worth keeping my internal logic.

  • the build works with CUDA, ROCM 5.3.x, MKL, SCI, OpenBLAS, cray-mpi, openmpi (and probably with mpich). The only dependencies that need more testing are QUIP, PEXSI, SuperLU. It is mostly a matter of detecting the libraries properly.
  • All others dependencies, libint2, libxsmm, libxc, cosma, elpa, dbcsr, SIRIUS, libvori (a missing function is showing up at link time) work fine provided that cmake can find them.
  • I added a debug mode with the possibility to turn off some of the GPU features. It is turned off by default
  • The VERSION is generated automatically but it will need some more work to distinguish between dev and official versions
  • More importantly there is NO modification of the source code
  • CP2K installation follows the default CP2K_PREFIX/{bin,lib,share}.
  • Data for pseudo potentials are stored in CP2K_PREFIX/share/data by default.
  • the compilation options still need some work.

Happy hacking.

@oschuett oschuett merged commit b0f1689 into cp2k:master Oct 27, 2022
@mtaillefumier mtaillefumier deleted the cmake_v6 branch January 25, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants