[rocfft][cufft] DFT update host task to use native command

[JackAKirk]

Description

Similar to #572 (see the discussions in that PR for technical details) except this covers fft backends for both amd and nvidia cases.

Update host task impl to use enqueue_native_command for DFT using the cuda/hip backends.

tests:

test_main_dft_ct_amd.txt
test_main_dft_rt_amd.txt
test_main_dft_rt_nvidia.txt
test_main_dft_ct_nvidia.txt

author: @hjabird

[JackAKirk]

@oneapi-src/onemkl-dft-write could you please review this?

Thanks

[Rbiessy]

Thanks for the PR! Note we have used this in 2 GROMACS benchmarks to get between 2% to 7% improvements on MI210.

[JackAKirk]

@anantsrivastava30 Is this OK to merge?

Thanks

[lhuot]

Why is it needed to duplicate this in both cuFFT and rocFFT backends and in various domains (BLAS, LAPACK, FFT)? Can't we have one wrapper used across all domains and backends?

[Rbiessy]

My understanding is that the domains have always been separated on purpose. This makes review much easier as any change affecting common code would technically require an approval from every domain owners.
I agree this could be discussed in an issue. To my knowledge there is very little code that could be common across domains, other than the types and exceptions which are already common.

[JackAKirk]

Hi @lhuot

I don't know, I am following the existing design. I think that it is a good question and it would be good to open this as an issue. However I don't think it is a good idea to block this PR for this, since this is a critical patch for Gromacs performance, which does not introduce this duplication that already exists in all other backends.

[lhuot]

Hi @lhuot

I don't know, I am following the existing design. I think that it is a good question and it would be good to open this as an issue. However I don't think it is a good idea to block this PR for this, since this is a critical patch for Gromacs performance, which does not introduce this duplication that already exists in all other backends.

Then, let's fix the code duplication in the DFT domain please.

[JackAKirk]

Hi @lhuot
I don't know, I am following the existing design. I think that it is a good question and it would be good to open this as an issue. However I don't think it is a good idea to block this PR for this, since this is a critical patch for Gromacs performance, which does not introduce this duplication that already exists in all other backends.

Then, let's fix the code duplication in the DFT domain please.

OK sure, do you want me to add a new header e.g. "execute_helper_generic.hpp" in oneMKL/src/dft/ with the fft_host_task implementation that is portable across hip and cuda backends, and then only include this header in the rocfft and cufft backends?
I can't include such a generic header in include/oneapi/mkl/detail/ because the enqueue_native_command extension is not available in intel backends and trying to link this header will break intel builds.

[lhuot]

Hi @lhuot
I don't know, I am following the existing design. I think that it is a good question and it would be good to open this as an issue. However I don't think it is a good idea to block this PR for this, since this is a critical patch for Gromacs performance, which does not introduce this duplication that already exists in all other backends.

Then, let's fix the code duplication in the DFT domain please.

OK sure, do you want me to add a new header e.g. "execute_helper_generic.hpp" in oneMKL/src/dft/ with the fft_host_task implementation that is portable across hip and cuda backends, and then only include this header in the rocfft and cufft backends? I can't include such a generic header in include/oneapi/mkl/detail/ because the enqueue_native_command extension is not available in intel backends and trying to link this header will break intel builds.

Sounds like a reasonable approach to me. Thanks!

[JackAKirk]

Hi @lhuot
I don't know, I am following the existing design. I think that it is a good question and it would be good to open this as an issue. However I don't think it is a good idea to block this PR for this, since this is a critical patch for Gromacs performance, which does not introduce this duplication that already exists in all other backends.

Then, let's fix the code duplication in the DFT domain please.

OK sure, do you want me to add a new header e.g. "execute_helper_generic.hpp" in oneMKL/src/dft/ with the fft_host_task implementation that is portable across hip and cuda backends, and then only include this header in the rocfft and cufft backends? I can't include such a generic header in include/oneapi/mkl/detail/ because the enqueue_native_command extension is not available in intel backends and trying to link this header will break intel builds.

Sounds like a reasonable approach to me. Thanks!

I've updated this now as requested. Tests still pass as posted in PR summary on both hip and cuda backends.

[lhuot]

LGTM, thanks!