-
Notifications
You must be signed in to change notification settings - Fork 59
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
CMake build system #131
CMake build system #131
Conversation
98286d0
to
fc26c79
Compare
CMakeLists.txt
Outdated
if(TARGET Kokkos::kokkos) | ||
message(STATUS "Using Kokkos settings for KokkosTools (source: ${Kokkos_SOURCE_DIR})") | ||
else() | ||
message(WARNING "Not using Kokkos for tools configuration\n\tNote: using settings from installed is Kokkos not yet supported") |
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.
Oh, this is a really slick idea
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 would there be a Kokkos::Kokkos
target w/o a find_package(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.
In my app's CMakeLists.txt
add_subdirectory(kokkos) # defines the target
add_subdirectory(kokkos-tools)
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.
That's not very clear, especially with the set(CMAKE_CXX_COMPILER ${Kokkos_CXX_COMPILER})
down below.
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, could definitely use a comment. It's also more of a Lawrence Livermore-ism than something Sandia codes do today
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 cleaned this up a bit (excuse messy draft). The idea is that:
- we check if Kokkos is built within same parent project - and before Tools, as in David's example above. Is there a better way than checking for
TARGET Kokkos::kokkos
? In this case we're all set up with options, compiler and flags. - we check for installed Kokkos in
Kokkos_ROOT
and extract all relevant settings from there. - in case Kokkos is nowhere found, we might just built Tools alone (figuring reasonable option defaults) or error out.
Any suggestions appreciated (where's the slicky part ?)
I like where you're starting with this. Also looping in @daboehme in case he has opinions about how we're playing with Caliper here |
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 my thoughts
CMakeLists.txt
Outdated
if(TARGET Kokkos::kokkos) | ||
message(STATUS "Using Kokkos settings for KokkosTools (source: ${Kokkos_SOURCE_DIR})") | ||
else() | ||
message(WARNING "Not using Kokkos for tools configuration\n\tNote: using settings from installed is Kokkos not yet supported") |
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 would there be a Kokkos::Kokkos
target w/o a find_package(Kokkos)
?
5b7e037
to
713ad68
Compare
4c285eb
to
baa6bd7
Compare
baa6bd7
to
82467e0
Compare
e83982a
to
44fd9b4
Compare
ea3e998
to
10792ac
Compare
10792ac
to
f557755
Compare
Thank you @jrmadsen for a thorough review and great comments! |
@crtrott @jrmadsen @daboehme @khuck @DavidPoliakoff @fnrizzi At this point I was able to draft a CMake build system for KokkosTools with:
You'll find more details in the PR description (just cleaned it up). I hope this shows overall idea about how things are connected, configured and built: any suggestions and ideas you may have will be much appreciated. My default plan is to continue with connecting the remaining tools same way. |
f557755
to
096f645
Compare
FYI: I think this looks really good. I think priority should be the NVIDIA connector tool and the CMake export. |
CMakeLists.txt
Outdated
cmake_policy(SET CMP0054 NEW) # simplify if() | ||
cmake_policy(SET CMP0057 NEW) # support IN_LIST in if() |
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.
These two are pointless if we require cmake 3.16
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.
Removed in PR.
CMakeLists.txt
Outdated
# Common settings | ||
option(BUILD_SHARED_LIBS "Build shared libraries" ON) | ||
if(WIN32) | ||
set(BUILD_SHARED_LIBS OFF) # We need to add __declspec(dllexport/dllimport) for Windows DLLs | ||
endif() |
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.
What is the intent here? BUILD_SHARED_LIBS
is a global flag recognized by CMake. It does not need to be an option. Was that an attempt to change the default value to ON
.
Also what is is the deal with Windows? What would be the point to build static?
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.
Regarding the option: yeah I think the idea was to have the default be ON.
And on Windows you can still link a static tools library into your app, and use the API to set callbacks. We may wanna actually set the KokkosTools_ENABLE_SINGLE default to ON for windows.
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.
If you want to get semi-lazy for windows and avoid all the import/export mess, just set CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to ON.
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.
Regarding the option: yeah I think the idea was to have the default be ON. And on Windows you can still link a static tools library into your app, and use the API to set callbacks. We may wanna actually set the KokkosTools_ENABLE_SINGLE default to ON for windows.
Yes, that makes sense. I will set KokkosTools_ENABLE_SINGLE (or KokkosTools_ENABLE_MONOLITHIC?) to ON.
endif() | ||
|
||
# Tools settings | ||
option(KokkosTools_ENABLE_SINGLE "Build single library interfacing all profilers and dispatching at runtime" OFF) |
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.
Did we discuss that name "single"? I suppose we can revisit later.
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.
not sure we discussed but yeah I agree we can revisit. MONOLITHIC might be another option? Or do it the other way around and name the option KokkosTools_ENABLE_SEPARATE_LIBS or so, which by default is ON?
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.
Using KokkosTools_ENABLE_MONOLITHIC makes most sense to me, JM2C.
mark_as_advanced(KokkosTools_REUSE_KOKKOS_COMPILER) | ||
|
||
# Fetch Kokkos options: | ||
acquire_kokkos_config() |
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.
When is this useful/desirable?
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 used for stuff like the connector tools to find the same CUDA library? Also there is some tools which have their own "ENABLE_CUDA" etc. e.g. the nvprof thing enabling is based on Kokkos_ENABLE_CUDA, and APEX use of CUPTI is based on the Kokkos option.
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 think so, I will dig in.
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.
The acquire_kokkos_config()
is used to show/print the kokkos configuration to the user, and to potentially set underlying (e.g.,CUPTI) tools options (e.g., ENABLE_CUDA).
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 can't find any connector tools right now making use of this from a quick look. I am leaving this line in for now.
Apple error and using Apple CMAKE variable
Apple Error message and using Apple CMAKE variable
More fixes for cleaner builds based on feedback from other Kokkos developers specifically @dalg24
Fixes in CMakeLists.txt based on feedback
Error out if Caliper or Apex not found , fix USE_MPI to KokkosTools_HAS_MPI (followingthe status message at the end of file printing value of USE_MPI and the naming convention established.). Notes: * Exit out of the build if Caliper and Apex are not found. * If MPI isn't found, the build just continues without MPI enabled.
…++11 -g -I$(VTUNE_HOME)/include LDFLAGS=-L$(VTUNE_HOME) directory
…kefiles putting back in makefile as fallback for CMakelists
Fixes #128
Summary
With this PR we enable building the tools with CMake. Caliper and Apex are connected as git submodules and can be built from source if no existing installation is found. In build, tools try to find Kokkos and propagate it's settings (e.g. enable CUDA support) to tools and TPLs. "Single library intefrace" is introduced which allows simple linking of all enabled tools and TPLs to client application.
Build: sample app CMake list file
Usage: sample app source code
Contents / Status
find_package(Kokkos)
based onKokkos_ROOT
);space-time-stack
,chrome-tracing
,simple-kernel-timer
,simple-kernel-timer-json
,memory-hwm-mpi
,memory-usage
,memory-events
,memory-hwm
andmemory-hwm-mpi
;kernel-filter
andkernel-logger
;builtin-ml-library
branch);KokkosTools::get_event_set(profiler_name, profiler_config)
single library interface:kernel-filter
andkernel-logger
) + implementstd::vector
/array overload ofKokkos::Tools::Experimental::set_callbacks()
for registering multiple event sets ?cmake_path(GET ... PARENT_PATH ...)
which requires CMake v3.20 ?Future considerations:
kokkosp_init_library()
called afterKokkos::initialize()
);