[DFT][cuFFT] Fix "Invalid strides" for shapes with a unit last dimension (#631)

[zjin-lcf]

Summary

Fixes #631.

oneapi::math::dft on the cuFFT backend throws dft/backends/cufft/commit: Invalid strides for multi-dimensional transforms whose last dimension has extent 1 (e.g. {3,1}, {2,1}, {5,1}, {2,3,1}). The same descriptors work on the Intel (MKL) backends, and the equivalent transform works when calling cuFFT directly.

Root cause

In src/dft/backends/cufft/commit.cpp, the backend uses std::min_element to find the dimension with the smallest stride (which becomes the cuFFT istride/ostride, i.e. the innermost dimension).

When a dimension has extent 1, its default stride ties with its neighbour's stride. std::min_element returns the first element among ties, so an outer dimension is mis-identified as the innermost one. This triggers the dimension-swap heuristic, producing a dimension/stride ordering that the check_stride_validity() pre-check then rejects — computing inner_n (e.g. 3) <= inner_stride (1) as false for both directions and throwing Invalid strides. The underlying cufftPlanMany call that would have been issued is actually valid.

(The related use of a_min after vec_a.erase(a_min) invalidated it was fixed separately in #750; this PR is rebased on top of that.)

Fix

• Search for the smallest stride in reverse, so ties resolve to the innermost (highest-index) dimension. This avoids the spurious dimension swap and lets valid transforms through.
• Factor the tie-breaking search into a small innermost_min helper used for both input and output strides.
• Add {3,1} and {2,3,1} to the DFT compute tests (test_params and test_params_real_in_place) as regression coverage.

No behavioural change for shapes without unit dimensions: for strictly decreasing default strides the reverse search selects the same (last) element as before.

Test plan

Validated end-to-end on an NVIDIA sm_103 device, CUDA 13.2 / cuFFT 12.2, using an open-source DPC++ toolchain (intel/llvm nightly with the CUDA backend). Built oneMath with -DENABLE_CUFFT_BACKEND=ON -DTARGET_DOMAINS=dft.

With the fix, the reproducer from #631 and related shapes all pass:

shape {3,1}   : OK -> [(3,0) (0,0) (0,0)]
shape {2,1}   : OK -> [(2,0) (0,0)]
shape {5,1}   : OK -> [(5,0) (0,0) (0,0) (0,0) (0,0)]
shape {2,3,1} : OK -> [(6,0) ...]
shape {3,4}   : OK   (regression check)
shape {2,3,4} : OK   (regression check)
shape {8}     : OK   (regression check)

• Confirmed direct cuFFT (cufftPlan2d(3,1) and the exact cufftPlanMany params oneMath generates) succeed on the B300 — the failure is in the pre-check, not cuFFT.
• Standalone comparison of the stride algorithm: original logic REJECTS {3,1}, {2,1}, {5,1}, {2,3,1}; fixed logic ACCEPTS them; regression shapes unchanged.
• End-to-end build + run on B300: all shapes above pass.
• After rebasing onto develop: built with -DENABLE_CUFFT_BACKEND=ON -DTARGET_DOMAINS=dft -DBUILD_FUNCTIONAL_TESTS=ON and ran test_main_dft_ct / test_main_dft_rt on a Tesla M40. Without the fix the 16 new non-USM {3,1}/{2,3,1} complex cases fail with Invalid strides; with the fix all non-USM tests pass. (USM tests fail on this particular machine with and without the change, independent of shape.)
• CI: DFT cuFFT functional tests.

[melonakos]

The #631 diagnosis here is correct and the fix is a good one, but half of this PR has already landed on develop via a different route — which is almost certainly why it now conflicts. A rebase should shrink it to something small and easy to approve.

What's already merged

#750, "[DFT][cuFFT] Fix use of invalidated iterator in commit.cpp", merged to develop and did the same thing as your second hunk. develop currently reads:

// erase() invalidates a_min (and any iterator after it).
const auto a_min_pos = a_min - stride_vecs.vec_a.begin();
stride_vecs.vec_a.erase(a_min);
stride_vecs.vec_b.erase(b_min);
...
if (a_min_pos != rank) {
    std::swap(n_copy[a_min_pos - 1], n_copy[rank - 1]);
}

That's your a_min_idx change under a different name. You were right about the bug — using a_min after erase(a_min) is UB — you just got there in parallel with whoever filed #750. This PR was opened 2026-07-28 and hasn't been rebased since, so it's carrying a now-redundant copy.

What's still genuinely needed

The #631 fix itself. develop still has the plain forward search:

auto a_min = std::min_element(stride_vecs.vec_a.begin() + 1, stride_vecs.vec_a.end());
auto b_min = std::min_element(stride_vecs.vec_b.begin() + 1, stride_vecs.vec_b.end());

std::min_element returns the first minimum, so when two dimensions share the smallest stride — which is exactly what happens when a dimension has extent 1 — the outer index wins, and the a_min - begin() != rank check below then performs a spurious dimension swap and rejects a perfectly valid transform. Shape {3,1} is the reproducer, and your comment explaining it is clear and worth keeping.

Your reverse-iterator search resolves ties to the highest index, which is the fix. I checked the iterator arithmetic: min_element over [make_reverse_iterator(end()), make_reverse_iterator(begin()+1)) walks from the last element down to begin()+1, and .base() - 1 maps the result back to the forward iterator for the same element. Correct.

So: please rebase onto develop and drop the duplicated hunk

That leaves just the min_element change plus its comment — a small, focused PR that I'd expect to go through quickly. It should also clear the conflict, and since no CI has ever run on this branch (GitHub can't build a merge commit for a conflicting PR), the rebase is what will finally get it tested.

Two suggestions while you're in there

Factor out the tie-breaking search. The expression is dense and appears twice verbatim. Giving it a name makes the intent legible at the call site:

// Smallest stride, preferring the innermost dimension on ties (see issue #631).
auto innermost_min = [](auto& v) {
    return std::min_element(std::make_reverse_iterator(v.end()),
                            std::make_reverse_iterator(v.begin() + 1)).base() - 1;
};
auto a_min = innermost_min(stride_vecs.vec_a);
auto b_min = innermost_min(stride_vecs.vec_b);

Add a {3,1} regression test. You've got a concrete failing shape from the issue — that's the cheapest possible test to write, and it's the thing that stops #631 coming back.

Related

#760 changes the corresponding validity logic on the rocFFT side, and I've approved it. Between these two the DFT backends should end up much closer to each other, which is a good outcome — the rocFFT file still had a "dft/backends/cufft" string in one of its throws, so they'd drifted a fair way apart.

[zjin-lcf]

Thanks @melonakos for the careful review. Addressed:

• Rebased onto develop and dropped the iterator hunk that is now in [DFT][cuFFT] Fix use of invalidated iterator in commit.cpp #750. The PR is a single commit touching only the min_element search.
• Factored the tie-breaking search into an innermost_min lambda, as suggested.
• Added {3,1} and {2,3,1} to test_params and test_params_real_in_place in compute_tests.cpp. Locally on cuFFT, the new complex cases fail with Invalid strides without the fix and pass with it. The in-place real cases, which were previously rejected as unimplemented (backward strides {0,1,1} tie), now run and pass.