Skip to content
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

Move default loop policy to meshblock. Closes #68 #252

Merged
merged 7 commits into from
Aug 22, 2020

Conversation

pgrete
Copy link
Collaborator

@pgrete pgrete commented Aug 5, 2020

PR Summary

This PR cleans up the default wrapper choices effectively removing one level in the nested hierarchy as the default pattern is now set at the MeshBlock level.
Moreover, the default pattern is now set by CMake rather than ifdefs in the header.

That the intermediate (now removed) wrappers were not actively used can also be seen by effectively only unit tests being affected by this change.
Already previously, the usage of pmb->par_for was encouraged as this is ensures that we use the correct execution space.

PR Checklist

  • Code passes cpplint
  • New features are documented. (does not apply)
  • Adds a test for any bugs fixed. Adds tests for new features. (nothing fixed, no interface change)
  • Code is formatted
  • Changes are summarized in CHANGELOG.md

@pgrete pgrete linked an issue Aug 5, 2020 that may be closed by this pull request
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good. There's a few places the abstraction leaks, which I'd like to close up.

src/CMakeLists.txt Show resolved Hide resolved
src/mesh/mesh.hpp Outdated Show resolved Hide resolved
src/kokkos_abstraction.hpp Show resolved Hide resolved
src/mesh/mesh.hpp Show resolved Hide resolved
src/reconstruct/plm_inline.hpp Outdated Show resolved Hide resolved
@@ -39,7 +39,8 @@ void DonorCellX1(parthenon::team_mbr_t const &member, const int k, const int j,
// compute L/R states for each variable
for (int n = 0; n <= nu; ++n) {
parthenon::par_for_inner(
member, il, iu, [&](const int i) { ql(n, i + 1) = qr(n, i) = q(n, k, j, i); });
DEFAULT_INNER_LOOP_PATTERN, member, il, iu,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #252 into develop will decrease coverage by 0.02%.
The diff coverage is 76.19%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #252      +/-   ##
===========================================
- Coverage    56.63%   56.60%   -0.03%     
===========================================
  Files           97       97              
  Lines        10504    10493      -11     
===========================================
- Hits          5949     5940       -9     
+ Misses        4555     4553       -2     
Impacted Files Coverage Δ
example/kokkos_pi/kokkos_pi.cpp 0.00% <0.00%> (ø)
example/advection/advection_package.cpp 98.35% <87.50%> (-0.02%) ⬇️
src/mesh/mesh.hpp 57.81% <88.88%> (+2.81%) ⬆️
src/kokkos_abstraction.hpp 98.90% <100.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2ea1ef...058e387. Read the comment docs.

@pgrete
Copy link
Collaborator Author

pgrete commented Aug 21, 2020

@Yurlungur I tried to address all your comments. This should be ready for re-review now.

@pgrete pgrete requested a review from Yurlungur August 21, 2020 21:35
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing all my nitpicks. Looks good now.

@Yurlungur Yurlungur merged commit c038b49 into develop Aug 22, 2020
@pgrete pgrete deleted the pgrete/move-default-loop-to-meshblock branch August 24, 2020 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Where to put default choices for Kokkos loop patterns?
3 participants