-
Notifications
You must be signed in to change notification settings - Fork 122
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
Add Kokkos unittests #826
Add Kokkos unittests #826
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
@kliegeois, does that set of test look good? My intent is to land several tests and then gradually incorporate #783. |
clang-tidy review says "All clean, LGTM! 👍" |
@vgvassilev I will review the tests later this week or early next week. |
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 have some comments on the examples.
unittests/Kokkos/ParallelFor.cpp
Outdated
Kokkos::View <double[1]> res("res"); | ||
res(0) = 0; | ||
Kokkos::parallel_for("polycalc", 5, KOKKOS_LAMBDA(const int i) { | ||
res(0) += pow(x, i+1)/(i+1); |
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.
This line includes data races.
Different threads (associated to different i
) write to the same memory address res(0)
.
This can be rewritten using a parallel_reduce
or using a parallel_for
with atomic adds.
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.
again, I understand that this is not a good way to use Kokkos. it was my intention to test this, because this should not break Clad for Kokkos. but I agree that a test for parallel_reduce
is needed as well.
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.
We should start with canonical examples where things work as suggested. I think that’s what Kim is saying. Once we get these things under control we can go for more exotic cases or things that are on the borderline in terms of parallel programming good practices.
unittests/Kokkos/parallel_sum.hpp
Outdated
using ViewTypeFlat = Kokkos::View< | ||
typename ViewtypeA::value_type*, Kokkos::LayoutRight, | ||
Kokkos::Device<typename ViewtypeA::execution_space, | ||
std::conditional_t<ViewtypeA::rank == 0, |
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.
@kliegeois I'm curious why the memory space changes depending on A's rank? I think ViewtypeA::memory_space
would work here for all possible ranks
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 am not sure why this is required.
I based that logic on the deep_copy
https://github.com/kokkos/kokkos/blob/master/core/src/Kokkos_CopyViews.hpp#L1311-L1317 .
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
@gojakuch, I was wondering what is the current state of this PR? Are all comments addressed? Are we close to merging? |
@vgvassilev I need to review it a last time. @gojakuch if you could avoid squashing commits and force pushing for the next PRs, that would be great to more easily see what has changed since the last review. Thanks! An alternative is to squash only when merging the PR. |
@kliegeois, thanks a lot, this is really helpful! |
@vgvassilev I think I addressed everything that had been commented the last time I pushed. I'm waiting for the last review from @kliegeois and I think we could merge after that.
alright. I honestly didn't think it's a big issue and thought that this is a good practice to just squash such little changes (I've been told so some time ago). sorry for the inconvenience. |
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.
Two last comments on my side. Thanks!
clang-tidy review says "All clean, LGTM! 👍" |
@vgvassilev I think we're ready to merge. |
@brian-kelley, is that good to go from your side, too? |
@gojakuch, I noticed that you chose a base branch a branch different than the master. I have corrected that in the PR. Can you rebase this PR against the master branch? |
Can you apply clang-format (as instructed in the output of the bot) and rebase this PR. Overall it looks good, the remaining test failures seem unrelated to it but I will need to investigate further. |
32e0e20
to
ce29f43
Compare
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #826 +/- ##
==========================================
+ Coverage 94.77% 94.80% +0.03%
==========================================
Files 49 49
Lines 7596 7606 +10
==========================================
+ Hits 7199 7211 +12
+ Misses 397 395 -2 |
We need to apply clang-format as suggested by the bot for multicommit setups... |
clang-tidy review says "All clean, LGTM! 👍" |
@vgvassilev I'm not sure if I understood you correctly. However, does this really matter if we're going to squash everything into a single commit "Add Kokkos unittests" when merging (as kliegeois once suggested)? at least, that's what I thought. the PR now passes all the checks except those we have talked about that I don't really understand and the clang-format / precheckin doesn't seem to be complaining. |
unittests/Kokkos/CMakeLists.txt
Outdated
@@ -13,4 +15,4 @@ if (NOT (LLVM_REQUIRES_RTTI OR LLVM_ENABLE_RTTI)) | |||
endif() | |||
|
|||
target_link_libraries(KokkosTests PUBLIC ${Kokkos_LIBRARIES}) | |||
target_include_directories(KokkosTests SYSTEM PRIVATE ${Kokkos_INCLUDE_DIRS}) | |||
target_include_directories(KokkosTests SYSTEM PUBLIC ${Kokkos_INCLUDE_DIRS}) |
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.
Does PRIVATE/PUBLIC change makes a difference?
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 really. you suggested it to fix building for me, but that turned out to be unrelated. we can revert it, if that's preferable.
f7c8e29
to
c064090
Compare
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
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.
LGTM! The failures in osx13 are quite puzzling to me. The problem came from the fact that we did not want to link libLLVM.so and added DISABLE_LLVM_LINK_LLVM_DYLIB
to add_unittest
that apparently somehow breaks osx13...
This adds unit tests for Kokkos'
parallel_for
,deep_copy
, andKokkos::View
accesses. Actual calls toclad::differentiate
andclad::gradient
are commented out, as the features are not implemented yet. This expands upon the tests in "Kokkos-aware Clad #783" (more tests for more features), improves readability (separate tests for forward and reverse modes) and matches the gtest guidelines more (no snake_case in test names, as_
is reserved for other purposes).