-
Notifications
You must be signed in to change notification settings - Fork 99
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
Overhaul of CMake build system to provide both TriBITS and standalone systems #491
Conversation
WIP - needs more testing to make sure rebase didn't break anything |
945bf07
to
893180c
Compare
@jjwilke what additional testing is needed before merge? Let me know if you'd like me to try some builds. |
Ideally we would test the same configurations that were previously tested with the raw makefiles? Not sure how feasible this is. I haven't had a chance to look at the current testing. |
@jjwilke I just pushed in an initial go of the
|
@jjwilke I ran into an issue with Cuda builds looking for cuBLAS, and have a question when installing Kokkos. With the change above pushed, if I try to configure kokkos-kernels with cuda enabled on kokkos-dev-2, I run into this error:
Edit: Here is my
|
IF (${LIB_TYPE} STREQUAL "INTERFACE_LIBRARY") | ||
# This is not an imported target | ||
# This an interface library that we created | ||
INSTALL( |
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.
@jjwilke I'm running into issues during configuration when hitting this INSTALL call with a Cuda build due to CUBLAS.
By default, CUBLAS is enabled when Cuda is enabled (I think this was preexisting behavior).
When this install call is hit I see errors like:
CMake Error at cmake/kokkoskernels_tpls.cmake:273 (INSTALL):
INSTALL TARGETS given target "KokkosKernels::CUBLAS" which is an alias.
Call Stack (most recent call first):
cmake/kokkoskernels_tpls.cmake:352 (KOKKOSKERNELS_EXPORT_IMPORTED_TPL)
cmake/kokkoskernels_tpls.cmake:437 (KOKKOSKERNELS_IMPORT_TPL)
CMakeLists.txt:371 (INCLUDE)
This is for TARGETS
set to KokkosKernels::CUBLAS
The ${CMAKE_INSTALL_BINDIR}
and ${CMAKE_INSTALL_LIBDIR}
are empty strings, though in the FindTPLCUBLAS module ${CUDA_CUBLAS_LIBRARIES}
returns the path to the cublas shared lib.
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 fix. I will make this more robust in 3.1. Stupid mistake on my part.
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.
Just tested to confirm it is working, thanks for fast fix!
Merge of #424 (and a necessary follow on PR right after) to reduce filename length for the generated cpp files for eti will require changes to
Lexicon:
@jjwilke I'll push the necessary updates above to this branch with a merge of develop and fix of the |
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.
Posted a couple questions regarding appended paths ${CMAKE_INSTALL_PREFIX}
during install, maybe irrelevant for stand-alone builds.
cmake/kokkoskernels_tribits.cmake
Outdated
INCLUDE(CMakePackageConfigHelpers) | ||
configure_package_config_file(cmake/KokkosKernelsConfig.cmake.in | ||
"${KokkosKernels_BINARY_DIR}/KokkosKernelsConfig.cmake" | ||
INSTALL_DESTINATION ${CMAKE_INSTALL_PREFIX}/lib/cmake/KokkosKernels) |
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.
On some systems (e.g. kokkos-dev-2) when I install kokkos, the appended path to ${CMAKE_INSTALL_PREFIX}
is lib64/cmake/KokkosKernels
instead of lib/cmake/KokkosKernels
. Is a change needed here to make this consistent between the two packages?
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.
Yeah, we should probably use the same trick with GNUInstallDirs
cmake/kokkoskernels_tribits.cmake
Outdated
INSTALL(FILES | ||
"${KokkosKernels_BINARY_DIR}/KokkosKernelsConfig.cmake" | ||
"${KokkosKernels_BINARY_DIR}/KokkosKernelsConfigVersion.cmake" | ||
DESTINATION ${CMAKE_INSTALL_PREFIX}/lib/cmake/KokkosKernels) |
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.
Same comment as above -
On some systems (e.g. kokkos-dev-2) when I install kokkos, the appended path to ${CMAKE_INSTALL_PREFIX}
is lib64/cmake/KokkosKernels
instead of lib/cmake/KokkosKernels
. Is a change needed here to make this consistent between the two packages?
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.
ditto - I can fix this quickly tonight
cmake/kokkoskernels_tribits.cmake
Outdated
"${KokkosKernels_BINARY_DIR}/KokkosKernelsConfigVersion.cmake" | ||
DESTINATION ${CMAKE_INSTALL_PREFIX}/lib/cmake/KokkosKernels) | ||
|
||
INSTALL(EXPORT KokkosKernelsTargets DESTINATION lib/cmake/KokkosKernels NAMESPACE Kokkos::) |
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.
Same comment as above -
On some systems (e.g. kokkos-dev-2) when I install kokkos, the appended path to ${CMAKE_INSTALL_PREFIX}
is lib64/cmake/KokkosKernels
instead of lib/cmake/KokkosKernels
. Is a change needed here to make this consistent between the two packages?
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.
ditto
c8122ed
to
f3e82cb
Compare
the |
KOKKOSKERNELS_IMPORT_TPL(CUBLAS) | ||
KOKKOSKERNELS_IMPORT_TPL(CUSPARSE) | ||
KOKKOSKERNELS_IMPORT_TPL(CUBLAS) | ||
KOKKOSKERNELS_IMPORT_TPL(CUBLAS INTERFACE) | ||
KOKKOSKERNELS_IMPORT_TPL(CUSPARSE) |
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.
Why doesn't CUSPARSE also have INTERFACE target property?
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.
It has to do with whether it's a single library or involves multiple libraries. You can only have a single IMPORTED_LOCATION
for an imported library.
I can now configure for Cuda builds on white and kokkos-dev-2 with the
|
Edit: Update - this is an issue in the Now I have build errors with Cuda kokkos-dev-2: SHAs: Environment
Configure:
Build errors:
|
Edit: Updated I didn't set |
Pushed a fix for the |
Cuda build on kokkos-dev-2 had link errors to CUBLAS
|
|
||
|
||
MACRO(kokkoskernels_create_imported_tpl NAME) | ||
CMAKE_PARSE_ARGUMENTS(TPL |
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.
@jjwilke I'm having link errors with CUBLAS in a Cuda build.
If I check output after the parse by adding this line:
MESSAGE(STATUS "AFTER PARSE: TPL_UNPARSED_ARGUMENTS = ${TPL_UNPARSED_ARGUMENTS}")
I see the following:
AFTER PARSE: TPL_UNPARSED_ARGUMENTS = LIBRARIES;/projects/sems/install/rhel7-x86_64/sems/compiler/cuda/9.2/base/lib64/libcublas.so
It looks like there is inconsistency in the FindTPLCUBLAS.cmake file when passing args to this marco, LIBRARIES
is passed in instead of LIBRARY
. If I change "LIBRARY" in the macro to "LIBRARIES" it parses but causes errors.
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.
Making the change in FindTPLCUBLAS to pass LIBRARY
using CUDA_CUBLAS_LIBRARY
as library path parses correctly and configures, will test out a rebuild.
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.
@jjwilke Same linking error with the change I mention above but the macro parse correctly. Should I push the change in to FindTPLCUBLAS (change "LIBRARIES" to "LIBRARY")?
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.
LIBRARIES
should be LINK_LIBRARIES
The TPL fix allowed my build to complete. Running
The issue is that The test is disabled in Cuda builds in the Makefile build system with a note that this is known to only run sequentially on host (same with trsv), but was re-enabled with these CMake updates. I'll open a separate issue as this is relevant to the Tpetra (and broader Trilinos) effort to move away from UVM requirements. |
7deaf7a
to
2b1c748
Compare
The failing tests should now be conditionally removed when UVM is not enabled. |
@ndellingwood wrote:
The |
Yes, along with the possibility this routine can be called as a free function from the host with Views or pointers to device data (which is what the unit test was doing). The View case can be checked easily by checking if the data is accessible on host. The tougher check is when a pointer to device data is passed to the function and |
@jjwilke On kokkos-dev-2 I tested with cusparse enabled and double+complex_double; everything configured and built successfully, but the following spgemm test had a runtime error:
Do you think the misaligned address message indicates that changes related to kokkos/kokkos#2255 had an impact here? Edit: My cm_generate_makefile configuration
|
Updated: I can confirm that after removing the |
@jjwilke looks like we're in a game of whack-a-mole with the TPL changes. I tried a clean rebuild of kokkos-kernels, serial, with user installs of blas and lapack and I am running into cmake configure issues: cmake-rebased SHA: b26e213
configure line: |
@ndellingwood Yep, got one of the lines wrong. Sorry about that. Missed testing the code path with custom libraries. Should be working now? |
@jjwilke working now, thanks for quick fix! |
I put in PR #503 with some fixes for the spgemm alignment issue that have come up so far. Testing is blocked on complex_float due to a bug in kokkos, filed an issue in kokkos/kokkos#2567. When the kokkos issue is resolved and #503 is merged I think we should be ready to merge this PR to allow for more extensive testing by others, @jjwilke would you be okay with that or prefer to hold off? |
Let's do it! I can rebase and remove conflicts. |
Added |
This script acts like, and will replace, the existing gnu-based generate_makefile script in order to configure makefiles for kokkos-kernels using the stand-alone CMake build system.
script for testing with the CMake build system
066fd9b
to
c4f5f2a
Compare
Issue #500 (with PR #503) and #506 (with PR in progress #507) needed before we can resume nightly testing against Kokkos' develop branch, but those issues are unrelated to this CMake PR - this PR just allows us to resume testing against Kokkos' develop branch which exposed the issues. Ready for merge @jjwilke , here goes! |
No description provided.