Repository navigation
fix(rocfft): include attach_halfN in the 2D twiddle table cache key - #11595
magaonka-amd merged 4 commits into
Conversation
Repo::GetTwiddles2D passes attach_halfN1 and attach_halfN2 to
twiddles_create_2D, where TwiddleTable2D uses them to append a half-N section to
the table, so they change both its length and its contents. They were absent
from repo_twd_key_2D_t, so a table generated for one setting of those flags was
handed back for the other.
A multi-dimensional real transform is computed by a complex kernel carrying
fused real pre/post-processing (EmbeddedType::Real2C_POST or C2Real_PRE) that
needs the half-N section, whereas an otherwise identical plain complex kernel
does not. When a complex transform of the same lengths and radices had been
planned earlier in the process and its plan was still alive, the real transform
was given that kernel's shorter table and returned silently wrong results, with
nothing reported by rocFFT or HIP. Whichever kernel was planned first won, so
the outcome depended on the order in which transforms ran.
On gfx950, a complex_forward of lengths {4, 4} followed by a real_inverse of
lengths {8, 4} while the first plan is still alive corrupts half the output
elements. The elements whose DFT terms carry only trivial factors stay exact,
which is the signature of a wrong twiddle table.
repo_twd_key_1D_t already includes attach_halfN, so this makes the 2D key
consistent with the 1D one.
Add a unit test that runs a multi-dimensional real transform with a clean
twiddle cache and again after such a complex transform, and requires the two
results to agree.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #11595 +/- ##
===========================================
+ Coverage 63.78% 64.19% +0.41%
===========================================
Files 2825 2825
Lines 469112 469114 +2
Branches 69265 69266 +1
===========================================
+ Hits 299193 301101 +1908
+ Misses 148872 146976 -1896
+ Partials 21047 21037 -10
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Thanks for this PR, @magaonka-amd . In fact, I think that it may be that the half_N2 may never be necessary (we don't really ever do the half-length complex sub-transform in any direction except for the Hermitian-symmetrized direction, which is always the first in row-major format). I agree that we should have some information for half_N(1) in the key for the map, though. |
Dropping attach_halfN2 as per review comments.
I should have noticed this caller more carefully, https://github.com/ROCm/rocm-libraries/blob/develop/projects/rocfft/library/src/tree_node_2D.cpp#L294 |
TheRock Submodule Bump ActivityNewest first
|
Summary of proposed changes:
Repo::GetTwiddles2Dpassesattach_halfN1/attach_halfN2totwiddles_create_2D, whereTwiddleTable2Duses them to append a half-N section to the table, so they change both its length and its contents. They were absent fromrepo_twd_key_2D_t, so a table generated for one setting of those flags was handed back for the other. Add them to the key and to itsoperator<.repo_twd_key_1D_talready includesattach_halfN, so this makes the 2D key consistent with the 1D one.Add
rocfft_UnitTest.twiddle_cache_2D_real_after_complex, which runs a multi-dimensional real transform with a clean twiddle cache and again after a complex transform of the colliding lengths whose plan is still alive, and requires the two results to agree.CHANGELOG entry under (Unreleased) rocFFT 1.0.40.
Impact
A multi-dimensional real transform runs on a complex kernel that also does the real pre/post-processing step. That step reads extra twiddle values which a plain complex kernel of the same size never uses. If such a complex transform was planned first and its plan is still alive, the real transform is handed the plain kernel's smaller table and quietly returns wrong numbers. Nothing fails, so there is no error to go on.
Whichever kernel is planned first wins, so the same code can be right or wrong depending on the order the transforms happen to run in. That is why the failure looks intermittent from the outside.
Affected: multi-dimensional real transforms, both forward (R2C) and inverse (C2R).
JIRA ID: ROCM-30556