IPPL-framework / ippl

IPPL is a C++ library to develop performance portable code for fully Eulerian, Lagrangian or hybrid Eulerian-Lagrangian methods.
https://ippl-framework.github.io/ippl/
GNU General Public License v3.0
21 stars 20 forks source link

Rewrite CMakeLists.txt #281

Closed manuel5975p closed 6 months ago

manuel5975p commented 7 months ago

Synopsis


Explanation of different sections of code:

if(NOT WIN32)
  string(ASCII 27 Esc)
  set(ColourReset "${Esc}[m")
  set(Red         "${Esc}[31m")
  set(Green       "${Esc}[32m")
  set(Yellow      "${Esc}[1;33m")
endif()

Sets a couple of color codes to highlight important output parts for warnings and hints.


if(NOT IPPL_PLATFORMS)
    message(STATUS "${Yellow}IPPL_PLATFORMS flag is not set, defaulting to serial backend${ColourReset}")
    set(IPPL_PLATFORMS "SERIAL")
endif()
if(IPPL_PLATFORMS)
    string(TOUPPER "${IPPL_PLATFORMS}" IPPL_PLATFORMS)
    #Handle
endif()

Defaults IPPL_PLATFORMS to SERIAL, also converts IPPL_PLATFORMS to uppercase to make options case insensitive.

srikrrish commented 7 months ago

Looks good in general. But I have some questions. How do the architecture for Kokkos or other Kokkos, Heffte options are set in this? Also it seems to always include Heffte without considering the ENABLE_FFT flag is that right?

felixschurk commented 7 months ago

I guess I should not make a review :D

srikrrish commented 7 months ago

I guess I should not make a review :D

Why not?.. Anyway if you have some comments feel free to tell.

manuel5975p commented 7 months ago

Heffte is neither looked for nor fetched if ENABLE_FFT is not set, the add_definitions line is not from me.

if (ENABLE_FFT)
    add_definitions (-DENABLE_FFT)
    find_package (Heffte 2.2.0 REQUIRED)
    find_package (Heffte 2.2.0 QUIET)
    if(NOT Heffte_FOUND)
        message(STATUS "Using FetchContent for Heffte")
        FetchContent_Declare(
             Heffte
             #DOWNLOAD_EXTRACT_TIMESTAMP
             GIT_REPOSITORY https://github.com/icl-utk-edu/heffte 
        )
        FetchContent_MakeAvailable(Heffte)
    endif()
    message (STATUS "Found Heffte_DIR: ${Heffte_DIR}")
endif ()

The Kokkos and Heffte architectures are set as follows:

    if("${IPPL_PLATFORMS}" STREQUAL "CUDA")
        message(STATUS "${Green}CUDA backend enabled${ColourReset}")
        set(Kokkos_ENABLE_CUDA ON CACHE BOOL "Enable Kokkos CUDA Backend")
        set(Heffte_ENABLE_CUDA ON CACHE BOOL "Set Heffte CUDA Backend")

# And so on...

Kokkos and Heffte are both configurable via cmake options if they are part of the project.

srikrrish commented 7 months ago

Heffte is neither looked for nor fetched if ENABLE_FFT is not set, the add_definitions line is not from me.

if (ENABLE_FFT)
    add_definitions (-DENABLE_FFT)
    find_package (Heffte 2.2.0 REQUIRED)
    find_package (Heffte 2.2.0 QUIET)
    if(NOT Heffte_FOUND)
        message(STATUS "Using FetchContent for Heffte")
        FetchContent_Declare(
             Heffte
             #DOWNLOAD_EXTRACT_TIMESTAMP
             GIT_REPOSITORY https://github.com/icl-utk-edu/heffte 
        )
        FetchContent_MakeAvailable(Heffte)
    endif()
    message (STATUS "Found Heffte_DIR: ${Heffte_DIR}")
endif ()

The Kokkos and Heffte architectures are set as follows:

    if("${IPPL_PLATFORMS}" STREQUAL "CUDA")
        message(STATUS "${Green}CUDA backend enabled${ColourReset}")
        set(Kokkos_ENABLE_CUDA ON CACHE BOOL "Enable Kokkos CUDA Backend")
        set(Heffte_ENABLE_CUDA ON CACHE BOOL "Set Heffte CUDA Backend")

# And so on...

Kokkos and Heffte are both configurable via cmake options if they are part of the project.

Ok thanks

manuel5975p commented 7 months ago

Addressed the comments, see here

srikrrish commented 7 months ago

Please modify the readme explaining how to do a serial, openmp and cuda builds. I can approve then.

manuel5975p commented 7 months ago

Readme is updated

manuel5975p commented 7 months ago

Duplication is removed

manuel5975p commented 7 months ago

So, whats the current concern?

srikrrish commented 7 months ago

So, whats the current concern?

Nothing from my side but I still see a few more unresolved threads. As Andreas mentioned it is also good to test this in a few different systems and confirm it is working. I can test it in Juwels CPU and GPUs until the end of today. @s-mayani Is it possible for you to test this on Perlmutter?

s-mayani commented 7 months ago

So, whats the current concern?

Nothing from my side but I still see a few more unresolved threads. As Andreas mentioned it is also good to test this in a few different systems and confirm it is working. I can test it in Juwels CPU and GPUs until the end of today. @s-mayani Is it possible for you to test this on Perlmutter?

Yes, I can test this on Perlmutter.

srikrrish commented 7 months ago

Also can we check out a specific SHA of Heffte with the cmake options? Because for the DCT reasons we have to use the Heffte master currently and that is not good in terms of reproducibility.

manuel5975p commented 7 months ago

I'll add the git hash later today, I know fetchcontent supports it.

srikrrish commented 7 months ago

I'll add the git hash later today, I know fetchcontent supports it.

That would be great! Thanks

manuel5975p commented 7 months ago

Okay, I tried the formerly error-generating configurations and it seems to work locally and on merlin. I also changed some lines in test/random/TestInverseTransformSamplingCustom.cpp since it generated a warning (due to being technically incorrect C++ code). It should also work for clang-based builds.

manuel5975p commented 7 months ago

I like the idea, but I still have a few questions. In addition to my comments, I would like to know

* How does one specify other Kokkos or HeFFTe flags when configuring Ippl?

* What happens when a Kokkos build or HeFFTe build fails? Will it abort the whole configure process?

To answer these question:

  1. By simply passing in the option that their CMakeLists.txt tests for
  2. Somewhat yes, however in cases where a target doesn't depend on e.g. Heffte, it's buildable even if Heffte fails. Heffte and Kokkos objects are just inserted into the make tree as if it was one project.
manuel5975p commented 7 months ago

@matt-frey it still says "matt-frey requested changes", are those changes addressed?

matt-frey commented 6 months ago

If a dependency does not build upon CMake, we cannot use this approach, right?

matt-frey commented 6 months ago

Does it work? I know that @srikrrish tried to compile and had several issues, are these resolved?

srikrrish commented 6 months ago

Does it work? I know that @srikrrish tried to compile and had several issues, are these resolved?

Yes it now works on Juwels GPU, OMP and serial builds. There still seem to be some warnings from serial build but even with build scripts they occur and unrelated to this cmake change. So there will be a new issue opened with regards to that.

manuel5975p commented 6 months ago

If a dependency does not build upon CMake, we cannot use this approach, right?

There are ways to handle non cmake-based, and at any rate find_package works regardless. In the absolute, absolute worst case, we're back at running a script from cmake again.