major cmake overhaul: #1902, #1930, #2026, #2027, #2099 #2104

crtrott · 2019-04-17T20:50:33Z

No description provided.

crtrott · 2019-04-18T02:07:30Z

this shoudldn't be merged in yet we need to do some testing.

cmake/kokkos_tribits.cmake

CMakeLists.txt

cmake/kokkos_settings.cmake

dalg24 · 2019-05-01T23:55:55Z

core/src/CMakeLists.txt

-CONFIGURE_FILE(kokkos.pc.in kokkos.pc @ONLY)
-INSTALL(FILES ${CMAKE_CURRENT_BINARY_DIR}/kokkos.pc DESTINATION lib/pkgconfig)
+#CONFIGURE_FILE(kokkos.pc.in kokkos.pc @ONLY)
+#INSTALL(FILES ${CMAKE_CURRENT_BINARY_DIR}/kokkos.pc DESTINATION lib/pkgconfig)


Did you mean to comment these?

Sort of? I should delete it here. It's in the kokkos_install.cmake file now.
Issue #2161 opened to address this.

I would prefer to revert these changes for now

dalg24 · 2019-05-01T23:56:57Z

example/global_2_local_ids/CMakeLists.txt


-SET(SOURCES "")
+SET(SOURCES G2L_Main.cpp)


I'd say don't even bother with the variable :)

dalg24 · 2019-05-10T15:59:57Z

cmake/KokkosConfig.cmake.in

+  ELSEIF(FOUND_HPX_ROOT)
+    SET(HPX_ROOT ${FOUND_HPX_ROOT})
+  ENDIF()
+ENDIF()


I find this block quite confusing. I suppose it is meant to help the downstream package to locate the same HPX that was found when building Kokkos.

Only the _DIR variable makes sense to me because AFAIU CMake sets it to the directory containing the configuration file provided by the package that was found. Also this seems to be the only invariant in the search procedure over the last few versions of CMake.

Also why would we do that for HPX and not for other dependencies that might have been enabled at the time Kokkos was built? Specifically I am thinking about MEMKIND, HWLOC, or Threads.

HPX is the only TPL right now loaded directly via find_package without a find module. CMake will attempt to load this downstream dependency automatically when a project does find_package(Kokkos) so we need to make sure CMake has the correct HPX_DIR

hwloc and memkind, as far as I can tell, are private dependencies. The headers/libraries should not be necessary to have downstream. If these were public, we would need to do some extra work to make sure the imported Kokkos::hwloc target can be found.

dalg24 · 2019-05-10T18:51:39Z

cmake/fake_tribits.cmake

+  TARGET_COMPILE_OPTIONS(${TARGET} ${ARGN})
+else()
+  TARGET_COMPILE_OPTIONS(${TARGET} ${ARGN})
+endif()


These used to be different. And then I changed tribits. And now they are same. Totally logical!

masterleinad · 2019-05-12T13:26:12Z

I also get

-- *****************************************************
-- 
CMake Error at cmake/kokkos_tribits.cmake:326 (TARGET_COMPILE_FEATURES):
  TARGET_COMPILE_FEATURES no known features for CXX compiler

  "GNU"

  version 7.3.0.
Call Stack (most recent call first):
  cmake/kokkos_tribits.cmake:250 (KOKKOS_INTERNAL_ADD_LIBRARY)
  CMakeLists.txt:125 (KOKKOS_MAKE_LIBKOKKOS)


-- Configuring incomplete, errors occurred!

with the current version using

cmake  
  -D KOKKOS_ARCH="Kepler35" \
  -D KOKKOS_ENABLE_SERIAL=ON \
  -D KOKKOS_ENABLE_OPENMP=ON \
  -D KOKKOS_ENABLE_CUDA=ON \
  -D KOKKOS_ENABLE_CUDA_LAMBDA=ON \
  -D CMAKE_CXX_COMPILER=${NVCC_WRAPPER} \
  -D CMAKE_BUILD_TYPE=Release \
  ..

dalg24 · 2019-05-12T17:13:18Z

@masterleinad It is a known issue with nvcc_wrapper. See the 1st part of Brad's comment

…IBRARIES

masterleinad · 2019-06-03T16:28:41Z

Using

cmake -DKOKKOS_ARCH=Volta70 \
      -DKOKKOS_ENABLE_CUDA=ON \
      -DKOKKOS_ENABLE_OPENMP=ON \
      -DKOKKOS_ENABLE_CUDA_LAMBDA=ON \
      -DCMAKE_CXX_COMPILER=/tmp/kokkos/bin/nvcc_wrapper \
      -DCMAKE_INSTALL_PREFIX=${PWD}/install \
      ${KOKKOS_PATH}

I currently get

/tmp/kokkos/bin/nvcc_wrapper   -I/tmp/kokkos/build_cmake/build/core/src -I/tmp/kokkos/core/src -I/tmp/kokkos/build_cmake/build  KOKKOS_CUDA_OPTIONS "-arch=sm_70>" "SHELL: -Xcompiler -fopenmp" -std=c++11 -std=gnu++11 -o CMakeFiles/kokkoscore.dir/impl/Kokkos_CPUDiscovery.cpp.o -c /tmp/kokkos/core/src/impl/Kokkos_CPUDiscovery.cpp
nvcc fatal   : Value 'sm_70>' is not defined for option 'gpu-architecture'

when building.

jjwilke · 2019-06-03T16:35:39Z

yes, type-o. This used to be a generator expression. If you want to continue, just delete the > from

    TARGET_COMPILE_OPTIONS(
      ${LIBRARY_NAME}
      PUBLIC ${KOKKOS_CUDA_OPTIONS}>
    )

in kokkos_tribits.cmake. I will fix in the next commit.

masterleinad · 2019-06-03T18:14:06Z

cmake/fake_tribits.cmake

+MACRO(GLOBAL_APPEND VARNAME)
+  SET(TEMP ${VARNAME})
+  LIST(APPEND TEMP ${ARGN})
+  GLOBAL_SET(${VARNAME} ${TEMP})


Suggested change

GLOBAL_SET(${VARNAME} ${TEMP})

SET(VARNAME "${VARNAME};${TEMP}" CACHE INTERNAL "")

This didn't work for me. I ended up with KOKKOS_CUDA_OPTIONS (literally) showing up in the compile arguments.

masterleinad · 2019-06-03T18:15:09Z

cmake/kokkos_tribits.cmake

+    )
+    SET(NODEDUP_CUDAFE_OPTIONS)
+    FOREACH(OPT ${NODEDEUP_CUDAFE_OPTIONS})
+      LIST(APPEND NODEDUP_CUDAFE_OPTIONS "SHELL: -Xcudafe ${OPT}") 


Suggested change

LIST(APPEND NODEDUP_CUDAFE_OPTIONS "SHELL: -Xcudafe ${OPT}")

LIST(APPEND NODEDUP_CUDAFE_OPTIONS -Xcudafe ${OPT})

Is this breaking your build? If you don't have the SHELL part, then CMake will deduplicate all the -Xcudafe flags, which you don't want.

Yes, it is breaking my build. I get

/tmp/kokkos/bin/nvcc_wrapper -I/tmp/kokkos/build_cmake/build/core/src -I/tmp/kokkos/core/src -I/tmp/kokkos/build_cmake/build -mavx "SHELL: -Xcompiler -fopenmp" -std=c++11 -std=gnu++11 -o CMakeFiles/kokkoscore.dir/impl/Kokkos_CPUDiscovery.cpp.o -c /tmp/kokkos/core/src/impl/Kokkos_CPUDiscovery.cpp g++: error: SHELL:: No such file or directory

What version of cmake are you using? Looks like I had my versions wrong. This didn't get added until 3.12, but I set minimum to 3.10.

@crtrott: Do we have any scenarios where we pass more than one -Xcudafe flag to the compiler? The only one I see right now is --diag_suppress=esa_on_defaulted_function_ignored. If we need to support more than one -Xcudafe we will need to bump minimum to 3.12.

I was using CMake 3.10.2.

masterleinad · 2019-06-03T18:15:27Z

cmake/kokkos_tribits.cmake

+  IF(KOKKOS_XCOMPILER_OPTIONS)
+    SET(NODEDUP_XCOMPILER_OPTIONS)
+    FOREACH(OPT ${KOKKOS_XCOMPILER_OPTIONS})
+      LIST(APPEND NODEDUP_XCOMPILER_OPTIONS "SHELL: -Xcompiler ${OPT}") 


Suggested change

LIST(APPEND NODEDUP_XCOMPILER_OPTIONS "SHELL: -Xcompiler ${OPT}")

LIST(APPEND NODEDUP_XCOMPILER_OPTIONS -Xcompiler ${OPT})

masterleinad · 2019-06-03T18:16:17Z

With these last changes, I can at least build Kokkos.

dalg24 · 2019-08-26T22:29:36Z

@masterleinad @jjwilke I am in favor to support finding Kokkos in the build directory but let's tackle that in a separate PR.

jjwilke · 2019-08-26T22:31:33Z

@masterleinad @jjwilke I am in favor to support finding Kokkos in the build directory but let's tackle that in a separate PR.

Using the cmake export(...) for build trees? Or some other mechanism?

masterleinad · 2019-08-27T14:00:19Z

@masterleinad @jjwilke I am in favor to support finding Kokkos in the build directory but let's tackle that in a separate PR.

Sure, it's not crucial.

crtrott · 2019-08-27T14:54:09Z

@masterleinad Daniel you got anything else problematic which needs to be resolved before we merge this finally?

crtrott · 2019-08-27T14:54:37Z

@jjwilke do you got more stuff we need to get in before merging?

jjwilke · 2019-08-27T15:15:46Z

I am done except for Trilinos testing. Features and documentation are complete. @dalg24?

The only sort of pending issue is #2161. pkgconfig has its idiosyncracies that don't map well to modern C++ (or projects mixing C/C++ for that matter). I don't know that we made a final decision on whether we want to install a .pc file. If not, I should probably remove all the pkgconfig stuff.

masterleinad · 2019-08-27T16:23:27Z

Daniel you got anything else problematic which needs to be resolved before we merge this finally?

Looking at the output of testing this pull request using ArborX (https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/ArborX/detail/PR-73/46/pipeline/20) we seem to miss linking against libcudart.so when using clang:

[...]
/opt/llvm/bin/clang++  -g  -mavx CMakeFiles/ArborX_DetailsTreeTraversal.exe.dir/tstDetailsTreeTraversal.cpp.o CMakeFiles/ArborX_DetailsTreeTraversal.exe.dir/utf_main.cpp.o  -o ArborX_DetailsTreeTraversal.exe -Wl,-rpath,/opt/boost/lib:/opt/openmpi/lib /opt/boost/lib/libboost_unit_test_framework.so /opt/kokkos/lib/libkokkos.a /usr/lib/x86_64-linux-gnu/libcuda.so /usr/lib/x86_64-linux-gnu/libdl.so -Wl,-rpath -Wl,/opt/openmpi/lib -Wl,--enable-new-dtags -pthread /opt/openmpi/lib/libmpi.so 
CMakeFiles/ArborX_DetailsTreeTraversal.exe.dir/tstDetailsTreeTraversal.cpp.o: In function `__cuda_register_globals':
tstDetailsTreeTraversal.cpp:(.text+0x1b8f): undefined reference to `__cudaRegisterVar'
tstDetailsTreeTraversal.cpp:(.text+0x1bcf): undefined reference to `__cudaRegisterVar'
[...]

masterleinad · 2019-08-27T19:08:40Z

Forcing the relevant libraries to be found via something like

diff --git a/cmake/kokkos_tribits.cmake b/cmake/kokkos_tribits.cmake
index a4a5c2d7..3353a007 100644
--- a/cmake/kokkos_tribits.cmake
+++ b/cmake/kokkos_tribits.cmake
@@ -257,12 +257,16 @@ FUNCTION(KOKKOS_LINK_TPLS LIBRARY_NAME)
     IF (KOKKOS_CXX_COMPILER_ID STREQUAL Clang)
        SET(LIB_cuda "-lcuda -lcudart")
        find_library( cuda_lib_ NAMES libcuda cuda HINTS ${KOKKOS_CUDA_DIR}/lib64 ENV LD_LIBRARY_PATH ENV PATH )
-       find_library( cudart_lib_ NAMES libcudart  cudart HINTS ${KOKKOS_CUDA_DIR}/lib64 ENV LD_LIBRARY_PATH ENV PATH )
+       find_library( cudart_lib_ NAMES libcudart cudart HINTS ${KOKKOS_CUDA_DIR}/lib64 ENV LD_LIBRARY_PATH ENV PATH )
        if (cuda_lib_)
           TARGET_LINK_LIBRARIES(${LIBRARY_NAME} PUBLIC ${cuda_lib_})
+       else()
+          MESSAGE(SEND_ERROR "libcuda is required but could not be found. Make sure to include it in your LD_LIBRARY_PATH.")
        endif()
        if (cudart_lib_)
           TARGET_LINK_LIBRARIES(${LIBRARY_NAME} PUBLIC ${cudart_lib_})
+       else()
+         MESSAGE(SEND_ERROR "libcudart is required but could not be found. Make sure to include it in your LD_LIBRARY_PATH.")
        endif()
     else()
        SET(LIB_cuda "-lcuda")

resolves that problem.

jjwilke · 2019-08-27T19:17:53Z

Forcing the relevant libraries to be found via something like

Ah, I see the code. So the issue is that we don't error out if the library is not found rather than a transitive dependency issue?

masterleinad · 2019-08-27T19:21:00Z

Ah, I see the code. So the issue is that we don't error out if the library is not found rather than a transitive dependency issue?

Yes, exactly.

masterleinad · 2019-08-27T21:11:48Z

As far as I am concerned, the pull request looks OK now.

crtrott · 2019-08-27T22:25:38Z

OK the error was a system issue when trying to run the clang-format test. This is fine. I gonna pull the trigger ..........

ipdemes · 2019-08-27T23:10:22Z

@jjwilke : is it possible to have KokkosConfig to contain "Kokkos_COMPILE_FLAGS" ?
Also, it seems like "Kokkos_INCLUDE_DIRS" is empty

jjwilke · 2019-08-28T00:51:08Z

@jjwilke : is it possible to have KokkosConfig to contain "Kokkos_COMPILE_FLAGS" ?
Also, it seems like "Kokkos_INCLUDE_DIRS" is empty

Kokkos_INCLUDE_DIRS needs to be deleted for the same reason we don't have a Kokkos_COMPILE_FLAGS. The necessary flags should be added transitively when you target_link_libraries(kokkos).

Do you have a special use case for needing Kokkos_COMPILE_FLAGS as a variable? It's always possible there's some use case that doesn't fit, but we just want to make sure it can't be done the "right way" with modern CMake.

ipdemes · 2019-08-29T20:23:40Z

@jjwilke : addind kokkos to the list of libraries when we call target_link_libraries(${target} .. didn't help: we don't get flags Kokkkos was compiled with in our CMAKE_CXX_FLAGS (like "-x cuda")

masterleinad · 2019-08-29T20:36:10Z

@ipdemes Do you see the correct flags in lib/CMake/Kokkos/KokkosTargets.cmake in INTERFACE_COMPILE_OPTIONS for kokkoscore and kokkoscontainers?
How exactly are you invoking target_link_libraries?

find_package(Kokkos REQUIRED)
target_link_libraries(ArborX INTERFACE Kokkos::kokkos)

works fine for us.

jjwilke · 2019-08-29T23:19:11Z

@ipdemes I'ill bounce this over to the issue tracker

ipdemes · 2019-08-30T17:02:14Z

@masterleinad : Yes I see correct flags in "INTERFACE_COMPILE_OPTIONS" for kokkoscore
@jjwilke :what is the issue #? It still doesn't work for me. I do exactly the same thing ....

ipdemes · 2019-08-30T17:17:11Z

@masterleinad @jjwilke :I have found the issue in our cmake set-up and it seems like Kokkos works now

jjwilke · 2019-08-30T21:16:47Z

@ipdemes #2264

jjwilke added 2 commits April 17, 2019 12:57

major cmake overhaul: #1902, #1930, #2026, #2027, #2099

c78c8ce

library list fix

38beedb

mhoemmen approved these changes Apr 17, 2019

View reviewed changes

jjwilke added 9 commits April 19, 2019 18:41

c++ standard now a compile feature, other fixes: #2103 #2099 #2100 #2025

6c107bd

tribits fixes, spack-friendly names

dd85bd5

cmake fixes for enable case issues: #2108

a4f0c4a

missing default option: #2108

bfe4656

tribits cleanup, tpl refactoring: #2110, #2098

961545f

test name fix

5ccaa57

tribits fixes for trilinos promotion

997fea5

more sweeping cmake changes: #2108, #2119, #2120

459092f

cxx flags and headers no longer use Makefile

75d3283

masterleinad mentioned this pull request May 10, 2019

Test Kokkos CMake overhaul arborx/ArborX#59

Closed

dalg24 reviewed May 10, 2019

View reviewed changes

This was referenced May 30, 2019

question about linking kokkos into existing CMake application #2159

Closed

Extension to CMake overhaul to completely avoid Makefile.kokkos #2160

Closed

jjwilke added 4 commits May 31, 2019 11:56

fixes for hwloc,memkind tpls

78a6477

avoid namespace targets to keep hwloc/memkind out of INTERFACE_LINK_L…

1991a8e

…IBRARIES

fixes for nvcc_wrapper and c++ standards: related to #2035

c1942c1

fixes for in-tree build

9f99b96

masterleinad reviewed Jun 3, 2019

View reviewed changes

dalg24 mentioned this pull request Jun 3, 2019

Option names for standalone CMake: all caps vs. camel case kokkos/kokkos-kernels#433

Closed

crtrott added 2 commits August 27, 2019 13:14

CMake: add error for cudart missing in link

2b0b562

Merge branch 'develop' into cmake-overhaul

d5c3623

crtrott merged commit d45e8ff into develop Aug 27, 2019

dalg24 mentioned this pull request Sep 10, 2019

Object files in the core/src directory interfere with CMake builds #2298

Closed

crtrott deleted the cmake-overhaul branch September 12, 2019 16:00

This was referenced Oct 4, 2019

Modify CMake to find hwloc, memkind, and librt #2098

Closed

Universal Kokkos find_package #2099

Closed

Link/Compiler flag propagation for Clang CUDA #2100

Closed

ndellingwood mentioned this pull request Jan 28, 2020

Kokkos + KokkosKernels Promotion To 2.9.99 trilinos/Trilinos#6671

Merged

ndellingwood mentioned this pull request Feb 18, 2020

Kokkos: Not rebuilding libkokkoscore correctly on rebuilds? trilinos/Trilinos#6855

Closed

	GLOBAL_SET(${VARNAME} ${TEMP})
	SET(VARNAME "${VARNAME};${TEMP}" CACHE INTERNAL "")

	LIST(APPEND NODEDUP_CUDAFE_OPTIONS "SHELL: -Xcudafe ${OPT}")
	LIST(APPEND NODEDUP_CUDAFE_OPTIONS -Xcudafe ${OPT})

	LIST(APPEND NODEDUP_XCOMPILER_OPTIONS "SHELL: -Xcompiler ${OPT}")
	LIST(APPEND NODEDUP_XCOMPILER_OPTIONS -Xcompiler ${OPT})


		SET(SOURCES "")
		SET(SOURCES G2L_Main.cpp)

major cmake overhaul: #1902, #1930, #2026, #2027, #2099 #2104

major cmake overhaul: #1902, #1930, #2026, #2027, #2099 #2104

Conversation

crtrott commented Apr 17, 2019

crtrott commented Apr 18, 2019

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

masterleinad commented May 12, 2019

dalg24 commented May 12, 2019

masterleinad commented Jun 3, 2019

jjwilke commented Jun 3, 2019

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

jjwilke Jun 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad commented Jun 3, 2019

dalg24 commented Aug 26, 2019

jjwilke commented Aug 26, 2019

masterleinad commented Aug 27, 2019

crtrott commented Aug 27, 2019 • edited

crtrott commented Aug 27, 2019

jjwilke commented Aug 27, 2019

masterleinad commented Aug 27, 2019

masterleinad commented Aug 27, 2019

jjwilke commented Aug 27, 2019

masterleinad commented Aug 27, 2019

masterleinad commented Aug 27, 2019

crtrott commented Aug 27, 2019

ipdemes commented Aug 27, 2019

jjwilke commented Aug 28, 2019

ipdemes commented Aug 29, 2019

masterleinad commented Aug 29, 2019

jjwilke commented Aug 29, 2019

ipdemes commented Aug 30, 2019

ipdemes commented Aug 30, 2019

jjwilke commented Aug 30, 2019

jjwilke Jun 3, 2019 •

edited

crtrott commented Aug 27, 2019 •

edited