Conversation
|
One difficulty is that the first 3-4 exercises are not perfect Kokkos code and can have issue when a Kokkos device backend is enabled. |
This way, the CMakeLists.txt is almost standard. Remove old common.cmake
advanced_reductions.cpp and parallel_scan.cpp cannot compile due to missing code (the exercise).
Use "" for include instead of <> to avoid adding `-I.` flag.
Still have to check on a Cuda computer.
This allows to not pollute the source directory.
This way we can promote the use of SHA256
|
@JBludau and @dalg24, I finally decided to follow your advice and I've factorized the code checking for a gpu. Furthermore, to apply the good practice described in the documentation to use either I can update the CI to use the new global CMake but I think it is better to leave it to a separate PR, this one being already too large. I do not understand the issue with Windows but it looks similar to the one of #107. |
| else () | ||
| if (EXISTS ${KokkosTutorials_KOKKOS_SOURCE_DIR}) | ||
| add_subdirectory(${KokkosTutorials_KOKKOS_SOURCE_DIR} Kokkos) | ||
| else () |
There was a problem hiding this comment.
Not blocking but do you know how does that play with multi-config generators?
There was a problem hiding this comment.
I will test and report it.
There was a problem hiding this comment.
It worked for my experiments.
|
So, creating a custom |
| else () | ||
| if (EXISTS ${KokkosTutorials_KOKKOS_SOURCE_DIR}) | ||
| add_subdirectory(${KokkosTutorials_KOKKOS_SOURCE_DIR} Kokkos) | ||
| else () |
There was a problem hiding this comment.
Not blocking but do you know how does that play with multi-config generators?
Co-authored-by: Paul Zehner <paul.zehner@alumni.enseeiht.fr>
Co-authored-by: Paul Zehner <paul.zehner@alumni.enseeiht.fr>
Co-authored-by: Paul Zehner <paul.zehner@alumni.enseeiht.fr>
…r19/cmake # Conflicts: # Exercises/tasking/Begin/CMakeLists.txt # Exercises/tasking/Solution/CMakeLists.txt
pzehner
left a comment
There was a problem hiding this comment.
I still have some concerns with the setup script.
Exercises/SetUpKokkos.cmake
Outdated
| # the default directory is inside the source tree. | ||
| # This might break if the default in source directory is called from multiple cmake instances at the same time. | ||
|
|
||
| set(KokkosTutorials_KOKKOS_SOURCE_DIR "dep/kokkos" CACHE PATH "Description for KokkosTutorials_KOKKOS_SOURCE_DIR") |
There was a problem hiding this comment.
Please update the help message.
Also, this downloads Kokkos in the build directory. One point of Damien was to use a common directory to not re-download it for every exercice, and I pretty much agree with him. In my experience though, since we have several projects cascading, it is tedious to do it right (maybe with CMAKE_CURRENT_LIST_DIR).
There was a problem hiding this comment.
Should be OK now.
Does not require CMake 3.14 anymore.
pzehner
left a comment
There was a problem hiding this comment.
Good job for updating the PR, I think we're almost there!
| set(CMAKE_BUILD_TYPE "${default_build_type}" CACHE STRING | ||
| "Choose the type of build, options are: Debug, Release, RelWithDebInfo and MinSizeRel." | ||
| FORCE) | ||
| endif () |
There was a problem hiding this comment.
Then, if Kokkos is already found, the build type is not specified for the files of the tutorial. Is it what we want?
There was a problem hiding this comment.
I've kept how it was working. I do not really see any added value and I am happy to remove this part.
There was a problem hiding this comment.
It's true that CMake always let the user choose what kind of build they want.
| # Where to find Kokkos' source code. This might be set by the user. | ||
| # In order to automatically share the download between exercises when they are compiled individually, | ||
| # the default directory is inside the source tree. | ||
| # This might break if the default in source directory is called from multiple cmake instances at the same time. |
There was a problem hiding this comment.
If several CMake instances download at the same time I am not it will always work.
There was a problem hiding this comment.
I see, maybe you can rework the sentence then.
| message(WARNING "This example requires CUDA, enable with -DKokkos_ENABLE_CUDA=ON") | ||
| return() |
There was a problem hiding this comment.
| message(WARNING "This example requires CUDA, enable with -DKokkos_ENABLE_CUDA=ON") | |
| return() | |
| message(FATAL_ERROR "This example requires CUDA, enable with -DKokkos_ENABLE_CUDA=ON") |
The code will not compile without Cuda.
There was a problem hiding this comment.
Oh wait, if you do a global build, this would cancel it completely.
There was a problem hiding this comment.
I am open to make it a fatal error but it means that we should protect the add_subdirectory.And potentially lose track of what is compiled or not.
There was a problem hiding this comment.
You can always do the check in the parent CMakeLists.txt and do not add_subdirectory. It sounds like code duplication, although not dramatic for me.
| message(WARNING "This example requires CUDA, enable with -DKokkos_ENABLE_CUDA=ON") | ||
| return() |
There was a problem hiding this comment.
| message(WARNING "This example requires CUDA, enable with -DKokkos_ENABLE_CUDA=ON") | |
| return() | |
| message(FATAL_ERROR "This example requires CUDA, enable with -DKokkos_ENABLE_CUDA=ON") |
There was a problem hiding this comment.
Oh wait, if you do a global build, this would cancel it completely.
|
There is also a missing include: diff --git a/Exercises/unique_token/Begin/unique_token.cpp b/Exercises/unique_token/Begin/unique_token.cpp
index 0af6bec..800a4f4 100644
--- a/Exercises/unique_token/Begin/unique_token.cpp
+++ b/Exercises/unique_token/Begin/unique_token.cpp
@@ -1,4 +1,5 @@
#include<Kokkos_Core.hpp>
+#include <iostream>
// EXERCISE: need to remove the ifdef...
#ifdef KOKKOS_ENABLE_OPENMP
|
Co-authored-by: Paul Zehner <paul.zehner@alumni.enseeiht.fr>
Co-authored-by: Paul Zehner <paul.zehner@alumni.enseeiht.fr>
JBludau
left a comment
There was a problem hiding this comment.
the rest seems reasonable
| Exercises/dep | ||
| *.o | ||
| *.host | ||
| *.cuda | ||
| KokkosCore_config.h | ||
| KokkosCore_config.tmp | ||
| *.a | ||
|
|
There was a problem hiding this comment.
I am in general against .gitignores if possible ... I want to see what changed and I think devs should be aware of what they push ...
There was a problem hiding this comment.
I think most of it can go out, besides Exercises/dep, since we're only relying on CMake. But I would add build*/.
I removed the .gitignore after a build to see what git status gives, and it's Kokkos sources and the build directories.
The goal is to provide a joint built for all exercises (and their solutions).
This way, compilation is straightforward for IDE users.
Currently, most of the exercises are built using the top-level CMake.
However, not all builds make sense, as CUDA builds are wrong for the first exercises. I have put configuration warnings when Kokkos is not configured correctly, as explaining why it is not working is still valuable.
I have also refactored the
fetch_contentimplementation:it is now called in an overloaded.find_packageto keepCMakeLists.txtas standard as possibleThe user can set three CMake variables to control the build system:
KokkosTutorials_KOKKOS_SOURCE_PATH: to specify a Kokkos source directory and by-passfetch_contentwhen building a custom Kokkos.I can improve the global CMake by usingexternal_projectfor the first exercises to "sandbox" their builds and allow a custom Kokkos.