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

Default host OpenMP? #897

Closed
pgrete opened this issue Jun 21, 2023 · 4 comments · Fixed by #896
Closed

Default host OpenMP? #897

pgrete opened this issue Jun 21, 2023 · 4 comments · Fixed by #896
Labels
question Further information is requested

Comments

@pgrete
Copy link
Collaborator

pgrete commented Jun 21, 2023

Currently, we enable OpenMP (threading) by default in Parthenon.
In AthenaPK (and most machine files), we disable it by default as we never really saw a good speedup versus flat MPI.

Any objection to change the default to (serial on the host)?
This would also allow to remove the PARTHENON_DISABLE_OPENMP parameter (and associated logic) and we could just rely on Kokkos_ENABLE_OPENMP.

ping @Yurlungur @jdolence @lroberts36 @brryan @bprather

@pgrete pgrete added the question Further information is requested label Jun 21, 2023
@Yurlungur
Copy link
Collaborator

So long as it is still possible to enable openmp kokkos backend this is fine for me. There's interest at LANL in using openmp, but I think having sensible defaults for optimal performance makes sense.

@lroberts36
Copy link
Collaborator

Yeah, I think this sounds good. I also have never seen particularly good OpenMP performance, so having serial by default seems better.

@bprather
Copy link
Collaborator

Very much in favor of this, and not for any speed-related reasons.

I just last week hit an issue where OpenMP had to be disabled (the Cray compiler wrapper on Frontier assumes OpenMP == OpenMP target). Having to disable it at each of the KHARMA, Parthenon, and Kokkos levels was a mess, so I'm happy having less logic around this in general.

The user knows whether they need OpenMP and already has Kokkos_ENABLE_OPENMP to say so, we should respect it.

@pgrete
Copy link
Collaborator Author

pgrete commented Jun 21, 2023

Thanks for the quick feedback.
I made the changes in #896

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants