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

Guard KOKKOS_ALL_COMPILE_OPTIONS if Cuda is not enabled #3387

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Guard KOKKOS_ALL_COMPILE_OPTIONS if Cuda is not enabled #3387

merged 2 commits into from
Sep 16, 2020

Conversation

finkandreas
Copy link
Contributor

@finkandreas finkandreas commented Sep 15, 2020

I'm building trilinos, not Kokkos standalone, and I'm building with a clang compiler, where I enable OpenMP as parallel hostspace, and no device space (i.e. Cuda disabled).

Variable KOKKOS_ALL_COMPILE_OPTIONS is referenced in file cmake/KokkosTrilinosConfig.cmake.in (when building with Trilinos, this triggers a bug)
The installed KokkosConfig.cmake will contain the lines:

  set_target_properties(Kokkos::kokkos PROPERTIES
    INTERFACE_LINK_LIBRARIES "kokkosalgorithms;kokkoscontainers;kokkoscore;-fopenmp"
    INTERFACE_COMPILE_FEATURES "cxx_std_14"
    INTERFACE_COMPILE_OPTIONS "$<$<COMPILE_LANGUAGE:CXX>:-fopenmp;-x;cuda>"
    INTERFACE_INCLUDE_DIRECTORIES "${KOKKOS_IMPORT_PREFIX}/include"
  )

When I want to use trilinos in another project and link to trilinos, which will pull in Kokkos::Kokkos, which adds the compile flags -x cuda. Guarding KOKKOS_ALL_COMPILE_OPTIONS works fine for me.

@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@dalg24 dalg24 requested a review from jjwilke September 15, 2020 11:48
@dalg24
Copy link
Member

dalg24 commented Sep 15, 2020

Alternative might be to guard

#------------------------------- KOKKOS_CUDA_OPTIONS ---------------------------
#clear anything that might be in the cache
GLOBAL_SET(KOKKOS_CUDA_OPTIONS)
# Construct the Makefile options
IF (KOKKOS_ENABLE_CUDA_LAMBDA)
IF(KOKKOS_CXX_COMPILER_ID STREQUAL NVIDIA)
GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS "-expt-extended-lambda")
IF(KOKKOS_COMPILER_CUDA_VERSION GREATER_EQUAL 110)
GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS "-Wext-lambda-captures-this")
ENDIF()
ENDIF()
ENDIF()
IF (KOKKOS_ENABLE_CUDA_CONSTEXPR)
IF(KOKKOS_CXX_COMPILER_ID STREQUAL NVIDIA)
GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS "-expt-relaxed-constexpr")
ENDIF()
ENDIF()
IF (KOKKOS_CXX_COMPILER_ID STREQUAL Clang)
SET(CUDA_ARCH_FLAG "--cuda-gpu-arch")
GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS -x cuda)
IF (KOKKOS_ENABLE_CUDA)
SET(KOKKOS_IMPL_CUDA_CLANG_WORKAROUND ON CACHE BOOL "enable CUDA Clang workarounds" FORCE)
ENDIF()
ELSEIF(KOKKOS_CXX_COMPILER_ID STREQUAL NVIDIA)
SET(CUDA_ARCH_FLAG "-arch")
ENDIF()
IF (KOKKOS_CXX_COMPILER_ID STREQUAL NVIDIA)
STRING(TOUPPER "${CMAKE_BUILD_TYPE}" _UPPERCASE_CMAKE_BUILD_TYPE)
IF (KOKKOS_ENABLE_DEBUG OR _UPPERCASE_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS -lineinfo)
ENDIF()
UNSET(_UPPERCASE_CMAKE_BUILD_TYPE)
IF (KOKKOS_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 9.0 AND KOKKOS_CXX_COMPILER_VERSION VERSION_LESS 10.0)
GLOBAL_APPEND(KOKKOS_CUDAFE_OPTIONS --diag_suppress=esa_on_defaulted_function_ignored)
ENDIF()
ENDIF()

The flag is added here

GLOBAL_APPEND(KOKKOS_CUDA_OPTIONS -x cuda)

Your solution looks fine to me but I want feedback from @jjwilke.
NOTE: Jenkins CI does not need to run for that PR.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

This only affects Trilinos so guarding in the Trilinos-only part looks sensible to me and mirrors what we do for a regular build in

IF (KOKKOS_ENABLE_CUDA)
TARGET_COMPILE_OPTIONS(
${LIBRARY_NAME}
PUBLIC $<$<COMPILE_LANGUAGE:CXX>:${KOKKOS_CUDA_OPTIONS}>
)
SET(NODEDUP_CUDAFE_OPTIONS)
FOREACH(OPT ${KOKKOS_CUDAFE_OPTIONS})
LIST(APPEND NODEDUP_CUDAFE_OPTIONS -Xcudafe ${OPT})
ENDFOREACH()
TARGET_COMPILE_OPTIONS(
${LIBRARY_NAME}
PUBLIC $<$<COMPILE_LANGUAGE:CXX>:${NODEDUP_CUDAFE_OPTIONS}>
)
ENDIF()
.

@finkandreas
Copy link
Contributor Author

But the problematic file is cmake/KokkosTrilinosConfig.cmake.in. This one, is a template and will ultimately install the file $INSTALLDIR/lib/cmake/kokkos/KokkosConfig.cmake. In this file the variable KOKKOS_ALL_COMPILE_OPTIONS is referenced. The compilation of trilinos works fine, but the installed cmake file is wrong.

@crtrott
Copy link
Member

crtrott commented Sep 15, 2020

This bug is in Trilinos right now? I.e. its in the release?

@finkandreas
Copy link
Contributor Author

I tested with Trilinos master branch, but looking at the source code of 13.0 release, it seems to be the same (did not try to compile it though)

@jjwilke
Copy link

jjwilke commented Sep 15, 2020

This looks fine. This is working for nvcc because KOKKOS_CUDA_OPTIONS is empty. There is one spot with clang where we add a flag to KOKKOS_CUDA_OPTIONS even if CUDA isn't enabled. This is why we haven't seen the error before (i.e. no one builds downstream Trilinos projects with clang-cuda).

I'm not sure if we care about consistent behavior, i.e. should KOKKOS_CUDA_OPTIONS always be empty if CUDA is not enabled? Or should KOKKOS_CUDA_OPTIONS contain all the expected CUDA options even if CUDA is not enabled. Super picky, but nice to be clean and consistent.

@crtrott
Copy link
Member

crtrott commented Sep 15, 2020

OK to test.

@dalg24
Copy link
Member

dalg24 commented Sep 16, 2020

Retest this please

@crtrott crtrott merged commit 3f9d820 into kokkos:develop Sep 16, 2020
@finkandreas finkandreas deleted the develop branch September 16, 2020 16:00
ndellingwood pushed a commit to ndellingwood/kokkos that referenced this pull request Sep 22, 2020
Guard KOKKOS_ALL_COMPILE_OPTIONS if Cuda is not enabled

(cherry picked from commit 3f9d820)
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

6 participants