Improve cmake #47

yasahi-hpc · 2024-02-12T07:36:39Z

Resolves #32

In this PR, we add the version check of Kokkos when building against a pre-installed Kokkos library.
Following functionalities are added.

Kokkos version check
KokkosFFT git version check
verbose git version in benchmark
verbose KokkosFFT in CMake build

yasahi-hpc · 2024-02-12T07:37:29Z

@cedricchevalier19 Hello, could you please review this PR?

pzehner · 2024-02-12T08:37:46Z

CMakeLists.txt

+    KokkosFFT
+    VERSION 0.0.0.0
+    LANGUAGES CXX
+)

 # Add cmake helpers for FFTW
 list(INSERT CMAKE_MODULE_PATH 0 "${CMAKE_CURRENT_SOURCE_DIR}/cmake")


You can simply:

list(PREPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

pzehner · 2024-02-12T08:39:17Z

CMakeLists.txt

 if (NOT KokkosFFT_INTERNAL_Kokkos)
    # First check, Kokkos is added as subdirectory or not
    if(NOT TARGET Kokkos::kokkos)
        find_package(Kokkos REQUIRED)
+        # Kokkos version check
+        include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/Kokkos_Version_Check.cmake)


Since you update CMAKE_CURRENT_SOURCE_DIR, you don't need to specify the absolute path anymore and can simply do:

include(Kokkos_Version_Check)

Do they work in both KokkosFFT as library and KokkosFFT as subdirectory cases?

This file is not included in the installation of Kokkos FFT, or is it?

Moreover, if you need a specific version of Kokkos, you should pass it as REQUIRED in find_package (if this is possible, see @cedricchevalier19 comment).

For build based on pre-installed Kokkos, this is included.

Moreover, if you need a specific version of Kokkos, you should pass it as REQUIRED in find_package (if this is possible, see @cedricchevalier19 comment).

As commented, I would like custom error messages.

pzehner · 2024-02-12T08:40:42Z

CMakeLists.txt

 if (NOT KokkosFFT_INTERNAL_Kokkos)
    # First check, Kokkos is added as subdirectory or not
    if(NOT TARGET Kokkos::kokkos)
        find_package(Kokkos REQUIRED)
+        # Kokkos version check


Maybe the Kokkos version check should be done unconditionally. I know it's unlikely that the embedded version comes outdated, but I think it doesn't hurt to have a straightforward approach.

Is there a way to access internal CMake variables in Kokkos CMake?

find_package can check for version directly no ?

If we rely on a pre-installed Kokkos yes. I am not sure how can I retrieve information from Kokkos added as a subdirectory. I added a specific function for finer error messages

I tested it with the sub-directory way and could access Kokkos_VERSION.

So, it would work except for using Kokkos as subdirectory of this KokkosFFT?

pzehner · 2024-02-12T08:41:29Z

CMakeLists.txt

@@ -44,6 +51,7 @@ endif()
 if(KokkosFFT_ENABLE_BENCHMARK)
    option(BENCHMARK_ENABLE_TESTING "Enable testing of the benchmark library." OFF)
    add_subdirectory(tpls/benchmark)
+    include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/KokkosFFT_Git_Hash.cmake)


You can do:

include(KokkosFFT_Git_Hash)

pzehner · 2024-02-12T08:42:11Z

CMakeLists.txt

@@ -60,6 +68,11 @@ set(
    KokkosFFT_Version_Info.hpp
 )

+# Set tpls
+set(KOKKOSFFT_TPL_LIST)
+include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/KokkosFFT_tpls.cmake)


You can do:

include(KokkosFFT_tpls)

cedricchevalier19

Just a quick review.

cedricchevalier19 · 2024-02-12T08:54:30Z

CMakeLists.txt

 math(EXPR KOKKOSFFT_VERSION_MAJOR "${KOKKOSFFT_VERSION} / 10000")
 math(EXPR KOKKOSFFT_VERSION_MINOR "${KOKKOSFFT_VERSION} / 100 % 100")
 math(EXPR KOKKOSFFT_VERSION_PATCH "${KOKKOSFFT_VERSION} % 100")


Please remove this, just alias to PROJECT_VERSION_* if it is not already done by CMake.

The question is that Kokkos versioning has numbers like "4.2.00" which seems to me PROJECT_VERSION_MAJOR. PROJECT_VERSION_MINOR.PROJECT_VERSION_PATCH.PROJECT_VERSION_TWEAK.

At the same time, it is reduced into "4.2.0" for version comparison. If we simply control PROJECT_VERSION_MAJOR. PROJECT_VERSION_MINOR.PROJECT_VERSION_PATCH, we can just alias version variables to these

cedricchevalier19 · 2024-02-12T08:55:33Z

CMakeLists.txt

 if (NOT KokkosFFT_INTERNAL_Kokkos)
    # First check, Kokkos is added as subdirectory or not
    if(NOT TARGET Kokkos::kokkos)
        find_package(Kokkos REQUIRED)
+        # Kokkos version check


find_package can check for version directly no ?

cedricchevalier19 · 2024-02-12T08:57:25Z

cmake/KokkosFFT_Git_Hash.cmake

Is this really useful? It is quite a lot of code for something that does not bring any feature.

It gives the git version in benchmark which should be important.

pzehner · 2024-02-12T09:06:01Z

CMakeLists.txt

+message("")
+message("-- KokkosFFT version: ${KOKKOSFFT_VERSION_MAJOR}.${KOKKOSFFT_VERSION_MINOR}.${KOKKOSFFT_VERSION_PATCH}")
+message("-- KokkosFFT TPLs")


The -- prefix is rendered by using the STATUS mode (see message documentation):

message(STATUS "KokkosFFT version: ${KOKKOSFFT_VERSION_MAJOR}.${KOKKOSFFT_VERSION_MINOR}.${KOKKOSFFT_VERSION_PATCH}") message(STATUS "KokkosFFT TPLs")

pzehner · 2024-02-12T09:30:07Z

cmake/KokkosFFT_tpls.cmake

@@ -0,0 +1,27 @@
+function(get_tpls_list tpls_list)


You should add a documentation in the header to describe what this function does.

pzehner · 2024-02-12T09:30:34Z

cmake/Kokkos_Version_Check.cmake

@@ -0,0 +1,11 @@
+function(check_minimum_required_kokkos kokkos_required_version)


You should add a documentation in the header to describe what this function does.

pzehner · 2024-02-12T09:36:46Z

CMakeLists.txt

+math(EXPR KOKKOSFFT_VERSION "${PROJECT_VERSION_MAJOR} * 10000 + ${PROJECT_VERSION_MINOR} * 100 + ${PROJECT_VERSION_PATCH} * 10 + ${PROJECT_VERSION_TWEAK}")
 math(EXPR KOKKOSFFT_VERSION_MAJOR "${KOKKOSFFT_VERSION} / 10000")
 math(EXPR KOKKOSFFT_VERSION_MINOR "${KOKKOSFFT_VERSION} / 100 % 100")
 math(EXPR KOKKOSFFT_VERSION_PATCH "${KOKKOSFFT_VERSION} % 100")


Do you really need to do that? You can use advantage of the standard version variables:

set(KOKKOSFFT_VERSION "${PROJECT_VERSION}") set(KOKKOSFFT_VERSION_MAJOR "${PROJECT_VERSION_MAJOR}") set(KOKKOSFFT_VERSION_MINOR "${PROJECT_VERSION_MINOR}") set(KOKKOSFFT_VERSION_PATCH "${PROJECT_VERSION_PATCH}")

Also, you specified a semver with tweak version (as of 0.0.0.0), and don't use it here.

I have never used tweak version, but Kokkos version seems to have that. #47 (comment)

The easiest solution is removing the tweak version.

pzehner · 2024-02-12T09:38:30Z

CMakeLists.txt

+# CMake Summary
+# ==================================================================
+
+message("")


Empty lines in CMake output is not common, in my experience.

This is to distinguish the summaries from Kokkos and KokkosFFT as shown.

-- Setting default Kokkos CXX standard to 17 -- Kokkos version: 4.2.0 -- The project name is: Kokkos -- Using internal gtest for testing -- Configured git information in /home/i18048/jh220031a/github/benchmark/github/kokkos-fft/build/generated/Kokkos_Version_Info.cpp -- Using -std=gnu++17 for C++17 extensions as feature -- Built-in Execution Spaces: -- Device Parallel: NoTypeDefined -- Host Parallel: Kokkos::OpenMP -- Host Serial: NONE -- -- Architectures: -- SKX -- Using internal desul_atomics copy -- Kokkos Devices: OPENMP, Kokkos Backends: OPENMP -- KokkosFFT version: 0.0.0 -- KokkosFFT TPLs FFTW_OPENMP

pzehner · 2024-02-12T09:38:45Z

CMakeLists.txt

+else()
+    message("   (None)")
+endif()
+message("")


Empty lines in CMake output is not common, in my experience.

pzehner · 2024-02-12T09:39:35Z

CMakeLists.txt

+if(KOKKOSFFT_TPL_LIST)
+    foreach(TPL ${KOKKOSFFT_TPL_LIST})
+        # [TO DO] show more information about the library (like location)
+        message("   ${TPL}")


message(STATUS " ${TPL}")

pzehner · 2024-02-12T09:40:01Z

CMakeLists.txt

+        message("   ${TPL}")
+    endforeach()
+else()
+    message("   (None)")


message(STATUS " (None)")

yasahi-hpc · 2024-02-12T10:58:32Z

@cedricchevalier19 @pzehner

Thank you for the reviews. I have fixed based on your reviews. I will merge this PR if you are fine.

Yuuichi Asahi added 2 commits February 10, 2024 17:04

Print git version in benchmark

4c40e94

Check Kokkos version when using pre-installed Kokkos

b089a20

yasahi-hpc requested a review from cedricchevalier19 February 12, 2024 07:36

pzehner reviewed Feb 12, 2024

View reviewed changes

cedricchevalier19 reviewed Feb 12, 2024

View reviewed changes

pzehner reviewed Feb 12, 2024

View reviewed changes

CMakeLists.txt Outdated

message(" ${TPL}")

endforeach()

else()

message(" (None)")

Copy link

Collaborator

pzehner Feb 12, 2024

Choose a reason for hiding this comment

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

message(STATUS " (None)")

Fix CMake based on comments

b12bca9

yasahi-hpc merged commit caf52c1 into main Feb 12, 2024
16 checks passed

yasahi-hpc deleted the improve-cmake branch February 12, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cmake #47

Improve cmake #47

yasahi-hpc commented Feb 12, 2024

yasahi-hpc commented Feb 12, 2024

pzehner Feb 12, 2024

pzehner Feb 12, 2024

yasahi-hpc Feb 12, 2024

pzehner Feb 12, 2024 •

edited

Loading

yasahi-hpc Feb 12, 2024

pzehner Feb 12, 2024 •

edited

Loading

yasahi-hpc Feb 12, 2024

cedricchevalier19 Feb 12, 2024

yasahi-hpc Feb 12, 2024 •

edited

Loading

pzehner Feb 12, 2024

yasahi-hpc Feb 12, 2024 •

edited

Loading

pzehner Feb 12, 2024

pzehner Feb 12, 2024

cedricchevalier19 left a comment

cedricchevalier19 Feb 12, 2024

yasahi-hpc Feb 12, 2024 •

edited

Loading

cedricchevalier19 Feb 12, 2024

cedricchevalier19 Feb 12, 2024

yasahi-hpc Feb 12, 2024

pzehner Feb 12, 2024 •

edited

Loading

pzehner Feb 12, 2024

pzehner Feb 12, 2024

pzehner Feb 12, 2024

yasahi-hpc Feb 12, 2024

pzehner Feb 12, 2024

yasahi-hpc Feb 12, 2024

pzehner Feb 12, 2024

pzehner Feb 12, 2024

pzehner Feb 12, 2024

yasahi-hpc commented Feb 12, 2024

		@@ -0,0 +1,11 @@
		function(check_minimum_required_kokkos kokkos_required_version)

Improve cmake #47

Improve cmake #47

Conversation

yasahi-hpc commented Feb 12, 2024

yasahi-hpc commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzehner Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzehner Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedricchevalier19 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzehner Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc commented Feb 12, 2024

pzehner Feb 12, 2024 •

edited

Loading

pzehner Feb 12, 2024 •

edited

Loading

yasahi-hpc Feb 12, 2024 •

edited

Loading

yasahi-hpc Feb 12, 2024 •

edited

Loading

yasahi-hpc Feb 12, 2024 •

edited

Loading

pzehner Feb 12, 2024 •

edited

Loading