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

Update Kokkos integration (and switch to default serial on host) #896

Merged
merged 11 commits into from
Jun 26, 2023

Conversation

pgrete
Copy link
Collaborator

@pgrete pgrete commented Jun 21, 2023

PR Summary

  • Move our existing scattered CMake Kokkos logic to one place
  • Removed PARTHENON_DISABLE_OPENMP option (and default to serial for host parallel). OpenMP can be enabled by Kokkos_ENABLE_OPENMP
  • Removed logic to enable CUDA lambda (default is on for Kokkos 4)
  • Removed explicit disable of deprecated Kokkos 3 code (as we're on 4 now)

Fixes #871
Fixes #897
Fixes #895

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@pgrete pgrete mentioned this pull request Jun 21, 2023
@pgrete pgrete changed the title WIP: Update Kokkos integration Update Kokkos integration (and switch to default serial on host) Jun 21, 2023
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.

Looks good to me. Love to see a negative line count PR. Doesn't this also resolve #895? ;)

CMakeLists.txt Show resolved Hide resolved
Copy link
Collaborator

@BenWibking BenWibking left a comment

Choose a reason for hiding this comment

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

Minor comments appended.

.github/workflows/check-compilers.yml Show resolved Hide resolved
message(WARNING
"Intel optimizer flags may not be passed through NVCC wrapper correctly. "
"If you encounter problems, please delete your CMake cache "
"and rerun CMake with -DTEST_INTEL_OPTIMIZATION=OFF.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an issue with the new LLVM-based Intel, or the old Intel compiler, or both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point.
If it's up to me I'd even remove these this and, if people actually use it, let them move it to a machine specific config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that would be cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any strong feelings here by anyone?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really.

doc/sphinx/src/building.rst Outdated Show resolved Hide resolved
src/bvals/comms/boundary_communication.cpp Show resolved Hide resolved
@pgrete
Copy link
Collaborator Author

pgrete commented Jun 22, 2023

Looks good to me. Love to see a negative line count PR. Doesn't this also resolve #895? ;)

Indeed, it does. Linking.

@pgrete
Copy link
Collaborator Author

pgrete commented Jun 26, 2023

Can we go ahead and merge?
If there are no additional comments by the end of today, I'll merge tomorrow (unless someone pulls the trigger earlier).

(I just briefly enabled and disabled auto-merge to trigger the full pipeline for testing)

@pgrete pgrete enabled auto-merge (squash) June 26, 2023 14:08
@pgrete pgrete disabled auto-merge June 26, 2023 14:08
@Yurlungur Yurlungur enabled auto-merge June 26, 2023 15:36
@Yurlungur Yurlungur merged commit d9d2212 into develop Jun 26, 2023
@pgrete pgrete deleted the pgrete/support-external-kokkos branch June 26, 2023 19:09
@pgrete pgrete mentioned this pull request Jun 27, 2023
4 tasks
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.

Default host OpenMP? Wrong clang for build check Kokkos options are set even when importing Kokkos
5 participants