Skip to content

feat(rocfft): Add support for 32/64 bit index arithmetic on Stockham Kernels - #11037

Merged
eng-flavio-teixeira merged 70 commits into
ROCm:developfrom
eng-flavio-teixeira:stockham_64_bit_indexing
Sep 16, 2026
Merged

eng-flavio-teixeira merged 70 commits into
ROCm:developfrom
eng-flavio-teixeira:stockham_64_bit_indexing

Conversation

@eng-flavio-teixeira

@eng-flavio-teixeira eng-flavio-teixeira commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Add the ability to switch between 32/64-bit integer arithmetic for the remaining kernels (Stockham, Bluestein, Real/Complex copy kernels, Twiddles, and Chirp).

JIRA ID: AIFFT-462

Technical Details

As previously implemented for the transpose kernel, the actual problem size is checked to decide the tightest integral type for the kernels involved in the computations.

A genuine effort has been made to determine if the integral types can be made 32-bit regardless of the FFT size, or if they need to be widened to 64-bit for large enough FFTs:

  • Integral types in the Stockham kernels were always 64-bit, but this is not really required if the FFT size is not large enough.
  • The real/complex copy kernels were hardcoded to 32-bit and they now can dynamically change depending on the FFT size.
  • Single kernel Bluestein kernels were inheriting the arithmetic from the base Stockham kernels, and now the integral type is also dynamic. Same for fused and non-fused Bluestein kernels.
  • Chirp and twiddle kernels were also fixed at 64-bit integral types and now are also dynamic, although they are not performance critical since they are on the plan path (not the hot FFT execution path).

This PR is IMO a step forward if, in the future, we decide to completely eliminate 64-bit integral types from the Stockham kernels, i.e., by passing an already offset pointer to memory in such a way that the kernel only needs 32-bit indexing to reach the work dataset. This is a larger change since it requires a complete redesign on how the kernels actually compute their indices work set.

There should be no observable performance changes for the FFTs we test. The Stockham kernels now have less register pressure on reasonably sized FFTs due to the switch to 32-bit arithmetic. The changes here, however, are not enough to affect occupancy - but, if callbacks or additional user computations are fused in the FFT kernels then there is more headroom to still maintain the same occupancy.

Other minor changes:

  • The previous type "index_type" has been renamed to a more suitable "integer_type" for the generated kernels, and IndexType now maps to KIntType.
  • Fixed an issue with the chirp generator that would produce incorrect results with sizes larger than 32-bits.

There is a pre-existing issue with large twiddle tables where, for example, a very large 1D, unbatched transform with length > 2^32

./rocfft-bench --length 8589934592 -t 0 --precision single

fail to run and throw an exception because the twiddle table look up calculation (TWLstep1 to TWLStep4) does not handle a length larger than 32 bit:

if(node.large1D > (size_t)256 * 256 * 256 * 256)
    throw std::runtime_error("large1D twiddle size too large error");

I will add a separate issue and fix for this one.

Test Plan

Existing tests should exercise the changes in this PR.

Test Result

The tests should pass without any issues. No performance regressions should be observed.

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Aug 19, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Aug 19, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@codecov

codecov Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.64312% with 88 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
projects/rocfft/library/src/include/rtc_kernel.h 35.00% 8 Missing and 5 partials ⚠️
...rojects/rocfft/library/src/rtc_stockham_kernel.cpp 74.47% 12 Missing ⚠️
projects/rocfft/library/src/rtc_test_harness.cpp 9.09% 10 Missing ⚠️
projects/rocfft/library/src/kargs.cpp 75.00% 4 Missing and 4 partials ⚠️
projects/rocfft/library/src/include/tree_node.h 82.50% 4 Missing and 3 partials ⚠️
...fft/library/src/device/generator/stockham_gen_cr.h 0.00% 5 Missing ⚠️
...fft/library/src/device/generator/stockham_pp_gen.h 0.00% 5 Missing ⚠️
...rojects/rocfft/library/src/rtc_realcomplex_gen.cpp 93.67% 5 Missing ⚠️
.../library/src/device/generator/stockham_pp_gen_cc.h 0.00% 4 Missing ⚠️
.../library/src/device/generator/stockham_pp_gen_rr.h 0.00% 4 Missing ⚠️
... and 7 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #11037      +/-   ##
===========================================
- Coverage    70.23%   69.63%   -0.60%     
===========================================
  Files         2810     2826      +16     
  Lines       462535   469975    +7440     
  Branches     68104    69410    +1306     
===========================================
+ Hits        324829   327246    +2417     
- Misses      114186   119011    +4825     
- Partials     23520    23718     +198     
Flag Coverage Δ *Carryforward flag
TensileLite-CPP 38.59% <ø> (-7.82%) ⬇️ Carriedforward from e29a94c
TensileLite-Unit 75.55% <ø> (-0.53%) ⬇️ Carriedforward from e29a94c
hipBLAS 90.62% <ø> (ø) Carriedforward from e29a94c
hipBLASLt 35.22% <ø> (-0.01%) ⬇️ Carriedforward from e29a94c
hipCUB 82.68% <ø> (ø) Carriedforward from e29a94c
hipDNN 86.96% <ø> (-0.06%) ⬇️ Carriedforward from e29a94c
hipFFT 43.11% <ø> (+0.04%) ⬆️
hipRAND 76.12% <ø> (ø) Carriedforward from e29a94c
hipSOLVER 68.92% <ø> (ø) Carriedforward from e29a94c
hipSPARSE 86.99% <ø> (ø) Carriedforward from e29a94c
rocBLAS 48.31% <ø> (ø) Carriedforward from e29a94c
rocFFT 51.82% <83.64%> (+4.66%) ⬆️
rocRAND 56.92% <ø> (+0.01%) ⬆️ Carriedforward from e29a94c
rocSOLVER 76.83% <ø> (ø) Carriedforward from e29a94c
rocSPARSE 74.61% <ø> (+<0.01%) ⬆️ Carriedforward from e29a94c
rocThrust 91.60% <ø> (ø) Carriedforward from e29a94c

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
projects/rocfft/library/src/chirp.cpp 82.05% <100.00%> (ø)
...library/src/device/generator/bluestein_generator.h 74.83% <100.00%> (+0.02%) ⬆️
...rocfft/library/src/device/generator/fftgenerator.h 83.96% <ø> (ø)
...rocfft/library/src/device/generator/stockham_gen.h 86.67% <100.00%> (+0.95%) ⬆️
...fft/library/src/device/generator/stockham_gen_2d.h 92.29% <100.00%> (+0.83%) ⬆️
...t/library/src/device/generator/stockham_gen_base.h 92.99% <100.00%> (+1.51%) ⬆️
...fft/library/src/device/generator/stockham_gen_cc.h 95.15% <100.00%> (-0.23%) ⬇️
...fft/library/src/device/generator/stockham_gen_rc.h 87.87% <100.00%> (+0.39%) ⬆️
...fft/library/src/device/generator/stockham_gen_rr.h 98.33% <100.00%> (ø)
...rocfft/library/src/device/kernel-generator-embed.h 100.00% <ø> (ø)
... and 35 more

... and 107 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@regan-amd
regan-amd self-requested a review September 2, 2026 23:02

@regan-amd regan-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the most significant feedback from a first pass (a deeper review will follow).

@regan-amd regan-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the tests. Some minor notes/suggestions but LGTM

Comment thread projects/rocfft/shared/ptrdiff.h Resolved
@eng-flavio-teixeira
eng-flavio-teixeira merged commit 0ef4e04 into ROCm:develop Sep 16, 2026
117 of 119 checks passed
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Abuudiii added a commit that referenced this pull request Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants