-
Notifications
You must be signed in to change notification settings - Fork 407
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
Set OpenMP flags according to host compiler #3127
Conversation
cmake/kokkos_compiler_id.cmake
Outdated
@@ -33,6 +33,21 @@ IF(INTERNAL_HAVE_COMPILER_NVCC) | |||
STRING(SUBSTRING ${TEMP_CXX_COMPILER_VERSION} 1 -1 TEMP_CXX_COMPILER_VERSION) | |||
SET(KOKKOS_CXX_COMPILER_VERSION ${TEMP_CXX_COMPILER_VERSION} CACHE STRING INTERNAL FORCE) | |||
MESSAGE(STATUS "Compiler Version: ${KOKKOS_CXX_COMPILER_VERSION}") | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we can't just set KOKKOS_CXX_HOST_COMPILER_ID
to CMAKE_CXX_COMPILER_ID
? These are same already, unless we override it to NVIDIA or HIP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that works and is simpler. 🙂
ENDIF() | ||
|
||
IF(INTERNAL_HAVE_COMPILER_NVCC) | ||
# Save the host compiler id before overwriting it. | ||
SET(KOKKOS_CXX_HOST_COMPILER_ID ${KOKKOS_CXX_COMPILER_ID}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only place the host variable gets set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we are only using it if the compiler is nvcc_wrapper
. In the end, we could also replace KOKKOS_CXX_HOST_COMPILER_ID
by KOKKOS_CXX_COMPILER_ID
globally or only define it locally if you feel strongly about it. At this point, I am fine with doing either.
@@ -5,26 +5,29 @@ SET(KOKKOS_CXX_COMPILER_ID ${CMAKE_CXX_COMPILER_ID}) | |||
SET(KOKKOS_CXX_COMPILER_VERSION ${CMAKE_CXX_COMPILER_VERSION}) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set:
SET(KOKKOS_CXX_HOST_COMPILER_ID ...)
here?
SET(KOKKOS_CXX_COMPILER_ID_BACKUP ${KOKKOS_CXX_COMPILER_ID}) | ||
SET(KOKKOS_CXX_COMPILER_ID ${KOKKOS_CXX_HOST_COMPILER_ID}) | ||
COMPILER_SPECIFIC_FLAGS( | ||
Clang -Xcompiler ${ClangOpenMPFlag} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead add an optional flag to the COMPILER_SPECIFIC_FLAGS
macro that takes an optional compiler ID which defaults to KOKKOS_CXX_COMPILER_ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a corresponding commit. I couldn't pass ${KOKKOS_CXX_HOST_COMPILER_ID}
directly, though, since that would conflict with parsing the compiler flags.
baad55b
to
6b36432
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have access to a PGI compiler, but presumably this has been confirmed to work with PGI.
Fixes #3125.