[DFT][rocFFT] Fix out-of-bounds stride handling

[zjin-lcf]

Summary

• avoid out-of-range std::array subscripts in rocFFT and DFT test stride handling, which abort 3-D transforms under checked standard libraries
• reject ranks above rocFFT's supported maximum instead of overflowing fixed-size storage
• correct forward/backward stride validation, packed-layout checks, and the rocFFT exception label

Test plan

• Run all rocFFT DFT functional tests on AMD Instinct MI300A with ROCm 7.2.4: 667 passed, 272 skipped, 0 failed
• Repeat the full suite with -D_GLIBCXX_ASSERTIONS: 667 passed, 272 skipped, 0 failed
• Verify baseline and fixed runs have identical skipped-test lists
• Verify 1-D, 2-D, and 3-D descriptor commits succeed under checked libstdc++; 4-D and 5-D commits throw unimplemented
• Run the pre-commit clang-format hook

[melonakos]

Good PR. I traced the validity logic against stride_helper.hpp and I'm satisfied it's correct — approving. A couple of notes below, one of which I'd like addressed either here or as a follow-up.

The valid_forward/valid_backward rewrite is right, and it fixes a real false-accept

This is the substance of the PR and it deserves more than a glance, because the old code looks plausible. From stride_vectors:

fwd_in(vec_a),
fwd_out(vec_b),
bwd_in(stride_choice == FB_STRIDES ? vec_b : vec_a),
bwd_out(stride_choice == FB_STRIDES ? vec_a : vec_b)

fwd_in is a reference bound to vec_a, so the old guard stride_vecs.fwd_in == stride_vecs.vec_a was tautologically true — it compared a vector with itself. That made the first disjunct unconditional and, worse, left the second disjunct (vec_b_valid_as_fwd_domain && vec_a_valid_as_bwd_domain) live. Since the bindings never assign the roles that way round, that branch could accept stride configurations that are genuinely invalid. Your version drops it.

I checked both API paths against the bindings and they match exactly:

• FB_STRIDES: bwd_in = vec_b (backward domain), bwd_out = vec_a (forward domain) — the same pair of requirements as forward, so ? valid_forward is correct.
• IO_STRIDES: bwd_in = vec_a (backward domain), bwd_out = vec_b (forward domain) — hence vec_a_valid_as_bwd_domain && vec_b_valid_as_fwd_domain. Correct.

What settled it for me: cuFFT already does exactly this on develop.

bool valid_forward = check_stride_validity(stride_vecs.fwd_in, stride_vecs.fwd_out);
bool valid_backward = stride_api_choice == dft::detail::stride_api::FB_STRIDES
                          ? valid_forward
                          : ...

So this isn't a new invention — it converges rocFFT onto the shape the cuFFT backend already has. The stray "dft/backends/cufft" string you fixed in the rocFFT throw is the fingerprint of the original copy-paste, and the two backends had drifted since. Anyone giving this a second review can validate it by diffing the two files.

are_strides_smaller_than_lengths

- domain_lengths[sindices[0]] <= svec[sindices[1]]
+ svec[sindices[0]] * domain_lengths[sindices[0]] <= svec[sindices[1]]

Correct, and it makes the first term consistent with the second, which already had the stride factor. The old form under-constrained the innermost dimension. Worth noting the blast radius is small: sindices is sorted ascending by stride, so svec[sindices[0]] is the smallest stride — for any contiguous layout it's 1 and old and new agree. Behavior only changes when the innermost stride is non-unit, which is exactly the case the old form got wrong.

The rest

• The dimensions > max_supported_dims guard is a genuine fix, not just defensive: std::copy of all dimensions into std::array<std::size_t, 3> lengths would have overflowed the array for a 4-D descriptor. Throwing unimplemented is the right response.
• &stride_a_indices[dimensions] is out-of-range subscripting when dimensions == 3 — UB even though it happens to yield the right address on every real implementation. begin() + dimensions is correct, and the same applies to the test_common.hpp change.
• Signed/unsigned loop counters: fine.

One request: add a regression test

This PR changes stride-validation semantics in two directions — it removes a false-accept path and tightens the innermost-stride condition — but adds no test exercising either. The only test-file change is the UB fix in test_common.hpp.

I'd like a case that pins the false-accept: a stride configuration that develop accepts and this branch rejects. That's the behavior you're actually fixing, and without it a future refactor can silently reintroduce it. I recognize this can't run in CI — there's no AMD runner — but it documents intent and runs for anyone with the hardware.

I'm approving rather than blocking on that, since the change is a clear improvement over a buggy state and review capacity here is the scarce resource. Happy for the test to come as a follow-up.

Minor: the title undersells this

"Fix out-of-bounds stride handling" reads like a pure bounds fix, and the validity-logic rewrite is the more consequential change. Worth retitling so a second reviewer doesn't skim it as mechanical — that's how genuine semantic changes slip through.

[ndingle-arm]

Approved, although I support @melonakos suggestion of a follow-up PR to strengthen the testing around the issues that were fixed. In particular, adding tests for:

• A layout previously false-accepted but now rejected.
• A non-unit innermost stride requiring the restored stride multiplier.
• Rank 4 rejecting with unimplemented.

Code assessment

The changes are technically sound:

• Rank >3 is rejected before copying into fixed-size storage.
• Iterator endpoints replace out-of-range &array[size] subscripting.
• The packed-layout calculation correctly includes the innermost stride.
• Forward/backward validity now matches the stride_vectors bindings for both stride APIs.
• The erroneous cuFFT exception label is corrected.
• git diff --check passes.