Rebase on KokkosFFT - Githubissues

CExA-project / ddc

DDC is a discrete domain computation library.

https://ddc.mdls.fr

Other

33 stars 5 forks source link

Rebase on KokkosFFT #655

Closed tpadioleau closed 1 month ago

tpadioleau commented 1 month ago

closes #502

based on #544

KokkosFFT requests:

[x] remove CMakeLists.txt from installed headers
[x] allow to install with --prefix
[x] allow to pass const Views
[x] workaround gcc -Warray-bounds warning defect
[x] how to know if KokkosFFT_ENABLE_HOST_AND_DEVICE is available ? rely on TPL variables
[x] wait for release v0.2.1

yasahi-hpc commented 1 month ago

LGTM. I have two comments.

In fft.hpp, it is not clear what are APIs. I would recommend to introduce Impl namespace and move all implementation details inside it.
In test.cpp, it seems that both norm and fft are not tested for GPUs. Can we rely on Kokkos::DefaultExecutionSpace and/or Kokkos::DefaultHostExecutionSpace? For example,

test_fft_norm<
            Kokkos::DefaultExecutionSpace,
            Kokkos::DefaultExecutionSpace::memory_space,
            float,
            Kokkos::complex<float>,
            RDimX>(ddc::FFT_Normalization::OFF);

tpadioleau commented 1 month ago

LGTM. I have two comments.

In fft.hpp, it is not clear what are APIs. I would recommend to introduce Impl namespace and move all implementation details inside it.

Do you mean that we rename ddc::detail to ddc::Impl ? Unless we missed some functions, internals should be in the ddc::detail namespace.

In test.cpp, it seems that both norm and fft are not tested for GPUs. Can we rely on Kokkos::DefaultExecutionSpace and/or Kokkos::DefaultHostExecutionSpace?

I think we do actually, see https://github.com/CExA-project/ddc/blob/1bbc983c2a1f55f5dee7e89ce43daebea4858ac4/tests/fft/fft.cpp#L362

Did you identify a missing variant ?

yasahi-hpc commented 1 month ago

LGTM. I have two comments.

In fft.hpp, it is not clear what are APIs. I would recommend to introduce Impl namespace and move all implementation details inside it.

Do you mean that we rename ddc::detail to ddc::Impl ? Unless we missed some functions, internals should be in the ddc::detail namespace.

I got it. Then, it is good for me.

In test.cpp, it seems that both norm and fft are not tested for GPUs. Can we rely on Kokkos::DefaultExecutionSpace and/or Kokkos::DefaultHostExecutionSpace?

I think we do actually, see

https://github.com/CExA-project/ddc/blob/1bbc983c2a1f55f5dee7e89ce43daebea4858ac4/tests/fft/fft.cpp#L362

Did you identify a missing variant ?

I just have a look at the difference and did not notice it is already there. If DDC does not support Threads backend, it is good.

tpadioleau commented 1 month ago

I just have a look at the difference and did not notice it is already there. If DDC does not support Threads backend, it is good.

Not yet indeed, there should not be much work to do to support it though. I will open an issue about it.