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.

Sign up for GitHub

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

Jump to bottom

[DFT][rocFFt] Address rocFFT failing tests #563

Merged
merged 7 commits into from
Sep 30, 2024
Merged

[DFT][rocFFt] Address rocFFT failing tests #563

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
092054b
[rocFFT] Work around bug in rocFFT for ROCm 6
hjabird Aug 22, 2024
54e0cf9
[rocFFT] Add checks for rocFFT version due to internal bug
s-Nick Aug 30, 2024
691b237
Remove debug printing point
s-Nick Aug 30, 2024
a9a65ac
Address PR comments
s-Nick Sep 2, 2024
fe8e133
Remove extra line
s-Nick Sep 2, 2024
aed1f63
Update namespace for cuFFT and portFFT descriptor
s-Nick Sep 12, 2024
e84a5b2
Add comment to explain current implementation status
s-Nick Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address PR comments 
Removing rocFFT version check during configurations.
Update checks on dft command for rocFFT version with known issue.
  • Loading branch information
@s-Nick
s-Nick committed Sep 2, 2024
commit a9a65acffa5b392b61a0d205a350dfd9502c55ea
8 changes: 0 additions & 8 deletions src/dft/backends/rocfft/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 3 additions & 9 deletions src/dft/backends/rocfft/commit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,9 @@ class rocfft_commit final : public dft::detail::commit_impl<prec, dom> {
// Link to rocFFT issue: https://github.com/ROCm/rocFFT/issues/507
if constexpr (rocfft_version_major == 1 && rocfft_version_minor == 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if-statement could be converted to #if directive. I preferred to use this style instead, but I am open to change it

(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) {
Copy link

Choose a reason for hiding this comment

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

Are we certain this is limited to dimensions > 2? Is a 2D case like
lengths = {A, B}
inStrides = {B 1}
outStrides = {1, A}
present in the test base?
Possibly relevant as well are 1D batch cases like
length = N;
batch = M;
inStrides = 1; inDistance = N;
outStrides = M; outDistance = 1;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review @raphael-egan
The 2D configuration above is not directly tested. If added, rocfft backend skips the tests with the following output:

Skipping test because: "oneMKL: DFT/commit: function is not implemented rocfft requires a stride to be divisible by all dimensions associated with smaller strides!"

If a configuration that respect inputs requirement is provided, tests are passed. Similar configurations are already in the test suite, so I don't think adding this test is a priority.

For 1D there isn't a test that reproduces what you suggested. I added it to check. It is computed correctly with the input above, although the outDistance is computed internally to perform the operation correctly. In any case, both configuration do not cause any issue with rocFFT.

Therefore, I wouldn't change the if-statement condition

Copy link

Choose a reason for hiding this comment

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

Given that the exception is thrown by this project and not by rocFFT itself, I would recommend to add a comment like
// rocFFT's functional status for problems like cfoA:B:1xB:1:A is unknown as of [commit hash] (due to project's implementation preventing testing thereof)

Unless an explicit mention of that as a known limitation is found in rocFFT's documentation/release notes (I could not find any), it looks to me that the check triggering this exception to be thrown is present here to ensure consistency of subsequent stride-related implementation steps (sorting operations, in particular).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, added comment in e84a5b2

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");
Rbiessy marked this conversation as resolved.
Show resolved Hide resolved
Expand Down