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

Introduce kokkosfft initialize #220

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Jan 13, 2025

Fixes #217

This PR aims at adding KokkosFFT::initialize and KokkosFFT::finalize in order to correctly handle setup functions which cannot be called more than once.

  • call setup functions of fftw and rocfft in KokkosFFT::initialize
  • call cleanup functions of fftw and rocfft in KokkosFFT::finalize
  • Add tests to ensure that KokkosFFT::initialize and KokkosFFT::finalize can only be called when Kokkos is available
  • Add KokkosFFT::initialize and KokkosFFT::finalize in examples

@yasahi-hpc yasahi-hpc requested a review from tpadioleau January 13, 2025 11:07
@yasahi-hpc yasahi-hpc self-assigned this Jan 13, 2025
@yasahi-hpc yasahi-hpc added bug Something isn't working enhancement New feature or request labels Jan 13, 2025
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Did you consider leveraging Kokkos::push_finalize_hook() instead?

@yasahi-hpc
Copy link
Collaborator Author

Did you consider leveraging Kokkos::push_finalize_hook() instead?

Thank you, that is interesting. At least, I can use Kokkos::push_finalize_hook() inside KokkosFFT::finalize() to ensure that the cleanup is made only once inside Kokkos::finalize().

However, we also need to call fftw_init_threads() and rocfft_setup() before using FFT APIs. If I am not wrong, a hook for Kokkos::initialize() is not available in Kokkos

@dalg24
Copy link
Member

dalg24 commented Jan 13, 2025

I overlooked that you actually acquire resources as well. You are correct, Kokkos::push_finalize_hook() would not accommodate for this.
That said we could look into an approach where we initialize the first time around that these resources are needed.

I would encourage seriously looking at alternatives. I see the introduction of KokkosFFT::{initialize,finalize} as a last resort measure.

Copy link
Member

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2,15 +2,15 @@
#
# SPDX-License-Identifier: MIT OR Apache-2.0 WITH LLVM-exception

add_library(fft INTERFACE)
add_library(fft STATIC KokkosFFT_Core.cpp)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
add_library(fft STATIC KokkosFFT_Core.cpp)
add_library(fft KokkosFFT_Core.cpp)

I would not force it to be STATIC to allow people to use BUILD_SHARED_LIBS, https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html.

[[nodiscard]] bool KokkosFFT::is_finalized() noexcept { return g_is_finalized; }

void KokkosFFT::initialize() {
if (!(Kokkos::is_initialized() || Kokkos::is_finalized())) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!(Kokkos::is_initialized() || Kokkos::is_finalized())) {
if (!Kokkos::is_initialized()) {

Do we need to test is_finalized ?

@tpadioleau
Copy link
Member

I would encourage seriously looking at alternatives. I see the introduction of KokkosFFT::{initialize,finalize} as a last resort measure.

Oh really why ? Are both functions a problem or only Kokkos::finalize ?

@yasahi-hpc
Copy link
Collaborator Author

I overlooked that you actually acquire resources as well. You are correct, Kokkos::push_finalize_hook() would not accommodate for this. That said we could look into an approach where we initialize the first time around that these resources are needed.

I would encourage seriously looking at alternatives. I see the introduction of KokkosFFT::{initialize,finalize} as a last resort measure.

Makes sense. The alternative solution I have in mind is to rely on static local object which is called at the first time when KokkosFFT API is called (see 154c12d). Unfortunately, we decide not to employ this approach because it may be unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing calls to rocfft_setup()/rocfft_cleanup() for the rocfft backend
3 participants