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

Support in place transform #149

Merged
merged 18 commits into from
Oct 24, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

Resolves #61

This PR aims at supporting in-place transform.
Following changes are made.

  • Add a function are_aliasing to check if the input and output views are alias
  • Add a test function for are_aliasing
  • Improve get_extents function to compute extents for in-place case
  • Use in-place options for ROCM and SYCL in case of in-place transforms
  • Add tests for 1D and 2D in-place transforms

@yasahi-hpc yasahi-hpc self-assigned this Oct 4, 2024
@yasahi-hpc yasahi-hpc added the enhancement New feature or request label Oct 4, 2024
@yasahi-hpc yasahi-hpc force-pushed the support-in-place-transform branch 2 times, most recently from 48e8072 to 7e00da4 Compare October 22, 2024 08:39
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.

Can you confirm that fftw, cufft and hipfft internally do a check for an in-place execution ? No need to modify anything on our side ?

fft/unit_test/Test_Transform.cpp Show resolved Hide resolved
common/unit_test/Test_Helpers.cpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
fft/src/KokkosFFT_Plans.hpp Outdated Show resolved Hide resolved
common/unit_test/Test_Utils.cpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Transform.cpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Utils.hpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Utils.hpp Outdated Show resolved Hide resolved
common/unit_test/Test_Helpers.cpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Transform.cpp Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

Can you confirm that fftw, cufft and hipfft internally do a check for an in-place execution ? No need to modify anything on our side ?

Thank you for reviews

I expect that fftw3 and cufft are doing checks internally, but it is not very clear from documentations.
If I understand, all we need is just passing same pointers as input and output
hipfft in turn checks internally (see https://github.com/ROCm/hipFFT/blob/f9a5de3d5bfaa08b52d964bf6501478ee71398f3/clients/hipfft_params.h#L512-L516)

@tpadioleau
Copy link
Member

I expect that fftw3 and cufft are doing checks internally, but it is not very clear from documentations. If I understand, all we need is just passing same pointers as input and output hipfft in turn checks internally (see https://github.com/ROCm/hipFFT/blob/f9a5de3d5bfaa08b52d964bf6501478ee71398f3/clients/hipfft_params.h#L512-L516)

That is also my understanding reading cufft.

@yasahi-hpc
Copy link
Collaborator Author

@tpadioleau
May I have your second review please

@yasahi-hpc
Copy link
Collaborator Author

@tpadioleau
It seems that we need to keep pi in double precision.

@tpadioleau
Copy link
Member

@tpadioleau It seems that we need to keep pi in double precision.

Not precise enough otherwise ?

@yasahi-hpc
Copy link
Collaborator Author

@tpadioleau It seems that we need to keep pi in double precision.

Not precise enough otherwise ?

Maybe.
If they compute twiddles with double precision and cast them into single precision, we need to use pi in double precision

@yasahi-hpc
Copy link
Collaborator Author

@tpadioleau Seems OK now.
If you feel good, I will merge this

tpadioleau
tpadioleau previously approved these changes Oct 24, 2024
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.

Seems ok to me

fft/unit_test/Test_Utils.hpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Utils.hpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Utils.hpp Outdated Show resolved Hide resolved
fft/unit_test/Test_Utils.hpp Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

@tpadioleau Thank you for review. I think I have fixed accordingly

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.

ok for me

@yasahi-hpc
Copy link
Collaborator Author

I will merge this. Thank you again for your review

@yasahi-hpc yasahi-hpc merged commit d620636 into kokkos:main Oct 24, 2024
20 checks passed
@yasahi-hpc yasahi-hpc deleted the support-in-place-transform branch October 24, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support in-place transform
2 participants