From 092054bfb0fe388185ef82a61c29d8f20584c211 Mon Sep 17 00:00:00 2001 From: Hugh Bird Date: Thu, 22 Aug 2024 11:23:35 +0100 Subject: [PATCH 1/7] [rocFFT] Work around bug in rocFFT for ROCm 6 * rocFFT distributed with ROCm 6+ contains a bug meaning that the strides cannot be set on a plan description twice. * This commit works around this issue. --- src/dft/backends/rocfft/commit.cpp | 25 +++++++++++++++---------- src/dft/backends/rocfft/descriptor.cpp | 8 ++------ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/dft/backends/rocfft/commit.cpp b/src/dft/backends/rocfft/commit.cpp index 74f480720..cbca80958 100644 --- a/src/dft/backends/rocfft/commit.cpp +++ b/src/dft/backends/rocfft/commit.cpp @@ -259,12 +259,15 @@ class rocfft_commit final : public dft::detail::commit_impl { std::reverse(stride_vecs.vec_b.begin(), stride_vecs.vec_b.end()); stride_vecs.vec_b.pop_back(); // Offset is not included. - rocfft_plan_description plan_desc; - if (rocfft_plan_description_create(&plan_desc) != rocfft_status_success) { + rocfft_plan_description plan_desc_fwd, plan_desc_bwd; // Can't reuse with ROCm 6 due to bug. + if (rocfft_plan_description_create(&plan_desc_fwd) != rocfft_status_success) { + throw mkl::exception("dft/backends/rocfft", __FUNCTION__, + "Failed to create plan description."); + } + if (rocfft_plan_description_create(&plan_desc_bwd) != rocfft_status_success) { throw mkl::exception("dft/backends/rocfft", __FUNCTION__, "Failed to create plan description."); } - // plan_description can be destroyed afted plan_create auto description_destroy = [](rocfft_plan_description p) { if (rocfft_plan_description_destroy(p) != rocfft_status_success) { @@ -273,7 +276,9 @@ class rocfft_commit final : public dft::detail::commit_impl { } }; std::unique_ptr - description_destroyer(plan_desc, description_destroy); + description_destroyer_fwd(plan_desc_fwd, description_destroy); + std::unique_ptr + description_destroyer_bwd(plan_desc_bwd, description_destroy); std::array stride_a_indices{ 0, 1, 2 }; std::sort(&stride_a_indices[0], &stride_a_indices[dimensions], @@ -324,7 +329,7 @@ class rocfft_commit final : public dft::detail::commit_impl { if (valid_forward) { auto res = - rocfft_plan_description_set_data_layout(plan_desc, fwd_array_ty, bwd_array_ty, + rocfft_plan_description_set_data_layout(plan_desc_fwd, fwd_array_ty, bwd_array_ty, nullptr, // in offsets nullptr, // out offsets dimensions, @@ -339,7 +344,7 @@ class rocfft_commit final : public dft::detail::commit_impl { "Failed to set forward data layout."); } - if (rocfft_plan_description_set_scale_factor(plan_desc, config_values.fwd_scale) != + if (rocfft_plan_description_set_scale_factor(plan_desc_fwd, config_values.fwd_scale) != rocfft_status_success) { throw mkl::exception("dft/backends/rocfft", __FUNCTION__, "Failed to set forward scale factor."); @@ -347,7 +352,7 @@ class rocfft_commit final : public dft::detail::commit_impl { rocfft_plan fwd_plan; res = rocfft_plan_create(&fwd_plan, placement, fwd_type, precision, dimensions, - lengths.data(), number_of_transforms, plan_desc); + lengths.data(), number_of_transforms, plan_desc_fwd); if (res != rocfft_status_success) { throw mkl::exception("dft/backends/rocfft", __FUNCTION__, @@ -380,7 +385,7 @@ class rocfft_commit final : public dft::detail::commit_impl { if (valid_backward) { auto res = - rocfft_plan_description_set_data_layout(plan_desc, bwd_array_ty, fwd_array_ty, + rocfft_plan_description_set_data_layout(plan_desc_bwd, bwd_array_ty, fwd_array_ty, nullptr, // in offsets nullptr, // out offsets dimensions, @@ -395,7 +400,7 @@ class rocfft_commit final : public dft::detail::commit_impl { "Failed to set backward data layout."); } - if (rocfft_plan_description_set_scale_factor(plan_desc, config_values.bwd_scale) != + if (rocfft_plan_description_set_scale_factor(plan_desc_bwd, config_values.bwd_scale) != rocfft_status_success) { throw mkl::exception("dft/backends/rocfft", __FUNCTION__, "Failed to set backward scale factor."); @@ -403,7 +408,7 @@ class rocfft_commit final : public dft::detail::commit_impl { rocfft_plan bwd_plan; res = rocfft_plan_create(&bwd_plan, placement, bwd_type, precision, dimensions, - lengths.data(), number_of_transforms, plan_desc); + lengths.data(), number_of_transforms, plan_desc_bwd); if (res != rocfft_status_success) { throw mkl::exception("dft/backends/rocfft", __FUNCTION__, "Failed to create backward rocFFT plan."); diff --git a/src/dft/backends/rocfft/descriptor.cpp b/src/dft/backends/rocfft/descriptor.cpp index 83fdbe1dc..22f21590a 100644 --- a/src/dft/backends/rocfft/descriptor.cpp +++ b/src/dft/backends/rocfft/descriptor.cpp @@ -22,9 +22,7 @@ #include "oneapi/mkl/dft/detail/rocfft/onemkl_dft_rocfft.hpp" -namespace oneapi { -namespace mkl { -namespace dft { +namespace oneapi::mkl::dft::detail { template void descriptor::commit(backend_selector selector) { @@ -46,6 +44,4 @@ template void descriptor::commit( template void descriptor::commit( backend_selector); -} //namespace dft -} //namespace mkl -} //namespace oneapi +} //namespace oneapi::mkl::dft::detail From 54e0cf91910a6584e8c81c522f0a2417cb7a6e10 Mon Sep 17 00:00:00 2001 From: nscipione Date: Fri, 30 Aug 2024 11:15:19 +0100 Subject: [PATCH 2/7] [rocFFT] Add checks for rocFFT version due to internal bug Due to rocFFt internal bug https://github.com/ROCm/rocFFT/issues/507 Add cmake version checks based upon rocFFT version. Add exception to deal with faulty cases for affected rocFFT version. RocFFT versions taken from https://github.com/ROCm/rocFFT/blob/develop/CHANGELOG.md --- src/dft/backends/rocfft/CMakeLists.txt | 9 +++++++++ src/dft/backends/rocfft/commit.cpp | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/dft/backends/rocfft/CMakeLists.txt b/src/dft/backends/rocfft/CMakeLists.txt index 1380c8f0a..6b2aba09b 100644 --- a/src/dft/backends/rocfft/CMakeLists.txt +++ b/src/dft/backends/rocfft/CMakeLists.txt @@ -49,6 +49,14 @@ find_package(HIP REQUIRED) # Require the minimum rocFFT version matching with ROCm 5.4.3. find_package(rocfft 1.0.21 REQUIRED) +if (${rocfft_VERSION_MAJOR} EQUAL "1" AND ${rocfft_VERSION_MINOR} EQUAL "0" + AND ((${rocfft_VERSION_PATCH} GREATER "22") + AND (${rocfft_VERSION_PATCH} LESS "31") )) + message(WARNING "Due to a bug in rocFFT some tests fail with the version in\ + use. If possible use a version greater of 1.0.30 or less of 1.0.23. + Current rocFFT version ${rocfft_VERSION}") +endif() + target_link_libraries(${LIB_OBJ} PRIVATE hip::host roc::rocfft) # Allow to compile for different ROCm versions. See the README for the supported @@ -62,6 +70,7 @@ find_path( NO_DEFAULT_PATH REQUIRED ) + target_include_directories(${LIB_OBJ} PRIVATE ${rocfft_EXTRA_INCLUDE_DIR}) target_link_libraries(${LIB_OBJ} PUBLIC ONEMKL::SYCL::SYCL) diff --git a/src/dft/backends/rocfft/commit.cpp b/src/dft/backends/rocfft/commit.cpp index cbca80958..8eb9398c6 100644 --- a/src/dft/backends/rocfft/commit.cpp +++ b/src/dft/backends/rocfft/commit.cpp @@ -39,6 +39,7 @@ #include "rocfft_handle.hpp" #include +#include #include namespace oneapi::mkl::dft::rocfft { @@ -259,6 +260,28 @@ class rocfft_commit final : public dft::detail::commit_impl { std::reverse(stride_vecs.vec_b.begin(), stride_vecs.vec_b.end()); stride_vecs.vec_b.pop_back(); // Offset is not included. + // This workaround is needed due to a confirmed issue in rocFFT from version + // 1.0.23 to 1.0.30. Those rocFFT version correspond to rocm version from + // 5.6.0 to 6.3.0. + // Link to rocFFT issue: https://github.com/ROCm/rocFFT/issues/507 + if constexpr (rocfft_version_major == 1 && rocfft_version_minor == 0 && + (rocfft_version_patch > 22 && rocfft_version_patch < 31)) { + if (dom == dft::domain::COMPLEX && dimensions > 2) { + auto stride_checker = [&](const auto& a, const auto& b) { + for (ulong i = 0; i < dimensions; ++i) { + if (a[i] != b[i]) + return false; + } + return true; + }; + std::printf("hello\n"); + if (!stride_checker(stride_vecs.vec_a, stride_vecs.vec_b)) + throw oneapi::mkl::unimplemented( + "DFT", func, + "due to a bug in rocfft version in use, it requires fwd and bwd stride to be the same for COMPLEX out_of_place computations"); + } + } + rocfft_plan_description plan_desc_fwd, plan_desc_bwd; // Can't reuse with ROCm 6 due to bug. if (rocfft_plan_description_create(&plan_desc_fwd) != rocfft_status_success) { throw mkl::exception("dft/backends/rocfft", __FUNCTION__, From 691b237811e7b1f8587ce506855bf31b00973b88 Mon Sep 17 00:00:00 2001 From: nscipione Date: Fri, 30 Aug 2024 15:05:29 +0100 Subject: [PATCH 3/7] Remove debug printing point --- src/dft/backends/rocfft/commit.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/dft/backends/rocfft/commit.cpp b/src/dft/backends/rocfft/commit.cpp index 8eb9398c6..2446b4391 100644 --- a/src/dft/backends/rocfft/commit.cpp +++ b/src/dft/backends/rocfft/commit.cpp @@ -274,7 +274,6 @@ class rocfft_commit final : public dft::detail::commit_impl { } return true; }; - std::printf("hello\n"); if (!stride_checker(stride_vecs.vec_a, stride_vecs.vec_b)) throw oneapi::mkl::unimplemented( "DFT", func, From a9a65acffa5b392b61a0d205a350dfd9502c55ea Mon Sep 17 00:00:00 2001 From: nscipione Date: Mon, 2 Sep 2024 13:33:50 +0100 Subject: [PATCH 4/7] Address PR comments Removing rocFFT version check during configurations. Update checks on dft command for rocFFT version with known issue. --- src/dft/backends/rocfft/CMakeLists.txt | 8 -------- src/dft/backends/rocfft/commit.cpp | 12 +++--------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/dft/backends/rocfft/CMakeLists.txt b/src/dft/backends/rocfft/CMakeLists.txt index 6b2aba09b..8b5cfff30 100644 --- a/src/dft/backends/rocfft/CMakeLists.txt +++ b/src/dft/backends/rocfft/CMakeLists.txt @@ -49,14 +49,6 @@ find_package(HIP REQUIRED) # Require the minimum rocFFT version matching with ROCm 5.4.3. find_package(rocfft 1.0.21 REQUIRED) -if (${rocfft_VERSION_MAJOR} EQUAL "1" AND ${rocfft_VERSION_MINOR} EQUAL "0" - AND ((${rocfft_VERSION_PATCH} GREATER "22") - AND (${rocfft_VERSION_PATCH} LESS "31") )) - message(WARNING "Due to a bug in rocFFT some tests fail with the version in\ - use. If possible use a version greater of 1.0.30 or less of 1.0.23. - Current rocFFT version ${rocfft_VERSION}") -endif() - target_link_libraries(${LIB_OBJ} PRIVATE hip::host roc::rocfft) # Allow to compile for different ROCm versions. See the README for the supported diff --git a/src/dft/backends/rocfft/commit.cpp b/src/dft/backends/rocfft/commit.cpp index 2446b4391..a6b49f392 100644 --- a/src/dft/backends/rocfft/commit.cpp +++ b/src/dft/backends/rocfft/commit.cpp @@ -266,15 +266,9 @@ class rocfft_commit final : public dft::detail::commit_impl { // Link to rocFFT issue: https://github.com/ROCm/rocFFT/issues/507 if constexpr (rocfft_version_major == 1 && rocfft_version_minor == 0 && (rocfft_version_patch > 22 && rocfft_version_patch < 31)) { - if (dom == dft::domain::COMPLEX && dimensions > 2) { - auto stride_checker = [&](const auto& a, const auto& b) { - for (ulong i = 0; i < dimensions; ++i) { - if (a[i] != b[i]) - return false; - } - return true; - }; - if (!stride_checker(stride_vecs.vec_a, stride_vecs.vec_b)) + if (dom == dft::domain::COMPLEX && + config_values.placement == dft::config_value::NOT_INPLACE && dimensions > 2) { + if (stride_vecs.vec_a != stride_vecs.vec_b) throw oneapi::mkl::unimplemented( "DFT", func, "due to a bug in rocfft version in use, it requires fwd and bwd stride to be the same for COMPLEX out_of_place computations"); From fe8e1331e11294dc611ec868c06b8f1f6f399247 Mon Sep 17 00:00:00 2001 From: nscipione Date: Mon, 2 Sep 2024 15:54:14 +0100 Subject: [PATCH 5/7] Remove extra line --- src/dft/backends/rocfft/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/dft/backends/rocfft/CMakeLists.txt b/src/dft/backends/rocfft/CMakeLists.txt index 8b5cfff30..1380c8f0a 100644 --- a/src/dft/backends/rocfft/CMakeLists.txt +++ b/src/dft/backends/rocfft/CMakeLists.txt @@ -62,7 +62,6 @@ find_path( NO_DEFAULT_PATH REQUIRED ) - target_include_directories(${LIB_OBJ} PRIVATE ${rocfft_EXTRA_INCLUDE_DIR}) target_link_libraries(${LIB_OBJ} PUBLIC ONEMKL::SYCL::SYCL) From aed1f63fea977b65a5c2da785b5405e4d1ae8850 Mon Sep 17 00:00:00 2001 From: nscipione Date: Thu, 12 Sep 2024 13:49:15 +0100 Subject: [PATCH 6/7] Update namespace for cuFFT and portFFT descriptor Update namespace to make it consistent across all backends. --- src/dft/backends/cufft/descriptor.cpp | 8 ++------ src/dft/backends/portfft/descriptor.cpp | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/dft/backends/cufft/descriptor.cpp b/src/dft/backends/cufft/descriptor.cpp index d102164c2..bf26b600f 100644 --- a/src/dft/backends/cufft/descriptor.cpp +++ b/src/dft/backends/cufft/descriptor.cpp @@ -22,9 +22,7 @@ #include "oneapi/mkl/dft/detail/cufft/onemkl_dft_cufft.hpp" -namespace oneapi { -namespace mkl { -namespace dft { +namespace oneapi::mkl::dft::detail { template void descriptor::commit(backend_selector selector) { @@ -44,6 +42,4 @@ template void descriptor::commit( backend_selector); template void descriptor::commit(backend_selector); -} //namespace dft -} //namespace mkl -} //namespace oneapi +} //namespace oneapi::mkl::dft::detail diff --git a/src/dft/backends/portfft/descriptor.cpp b/src/dft/backends/portfft/descriptor.cpp index d72d23bb5..c45b9f2c5 100644 --- a/src/dft/backends/portfft/descriptor.cpp +++ b/src/dft/backends/portfft/descriptor.cpp @@ -22,7 +22,7 @@ #include "oneapi/mkl/dft/detail/portfft/onemkl_dft_portfft.hpp" -namespace oneapi::mkl::dft { +namespace oneapi::mkl::dft::detail { template void descriptor::commit(backend_selector selector) { @@ -44,4 +44,4 @@ template void descriptor::commit( template void descriptor::commit( backend_selector); -} // namespace oneapi::mkl::dft +} // namespace oneapi::mkl::dft::detail From e84a5b28c44e6823e5a08d1a06e40b3171858666 Mon Sep 17 00:00:00 2001 From: nscipione Date: Fri, 27 Sep 2024 11:28:29 +0100 Subject: [PATCH 7/7] Add comment to explain current implementation status --- src/dft/backends/rocfft/commit.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/dft/backends/rocfft/commit.cpp b/src/dft/backends/rocfft/commit.cpp index a6b49f392..991b0c471 100644 --- a/src/dft/backends/rocfft/commit.cpp +++ b/src/dft/backends/rocfft/commit.cpp @@ -266,6 +266,8 @@ class rocfft_commit final : public dft::detail::commit_impl { // Link to rocFFT issue: https://github.com/ROCm/rocFFT/issues/507 if constexpr (rocfft_version_major == 1 && rocfft_version_minor == 0 && (rocfft_version_patch > 22 && rocfft_version_patch < 31)) { + // rocFFT's functional status for problems like cfoA:B:1xB:1:A is unknown as + // of 4ed3e97bb7c11531684168665d5a980fde0284c9 (due to project's implementation preventing testing thereof) if (dom == dft::domain::COMPLEX && config_values.placement == dft::config_value::NOT_INPLACE && dimensions > 2) { if (stride_vecs.vec_a != stride_vecs.vec_b)