Repository navigation
test(rocfft): cover data_layout stride and offset arithmetic - #12667
Merged
Abuudiii merged 2 commits intoOct 26, 2026
Merged
Conversation
✅ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12667 +/- ##
===========================================
- Coverage 76.01% 75.94% -0.07%
===========================================
Files 2869 2869
Lines 475537 475537
Branches 71215 71215
===========================================
- Hits 361460 361131 -329
- Misses 89533 89922 +389
+ Partials 24544 24484 -60
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
regan-amd
reviewed
Oct 17, 2026
regan-amd
left a comment
regan-amd
left a comment
Contributor
There was a problem hiding this comment.
Due to our own delayed consideration for this PR, these changes are outdated now unfortunately: please rebase and update the tests as needed to reflect the current status of the code base.
Specifically:
- #8718 removed the
is_partialconstructor parameter fromdata_layout_t. The constructor is nowdata_layout_t(lower, upper, strides, batch_rank = 1), so every 5-argument call indata_layout_test.cppno longer compiles againstdevelop. - The same change removed the check that rejects a nonzero lower bound on a non-partial layout. The
EXPECT_THROW/EXPECT_NO_THROWpair that tests it (around lines 36-40) should be removed rather than adapted. The new check inget_other_inplace_layout_for(a nonzero lower bound on a length axis throwsstd::logic_error) could be a good replacement test.
Abuudiii
force-pushed
the
users/Abdullah/rocfft-internal-test-data-layout
branch
from
October 19, 2026 21:52
286e758 to
6168549
Compare
Contributor
Author
|
Thanks for the review. Rebased on current
Tested on gfx1201 (ROCm 7.2.4) across ~56 seeds: 15/15 data_layout tests, 19/19 total. I also checked that the tests catch bugs by breaking |
1 task done
evetsso
approved these changes
Oct 21, 2026
Replace the hand-written --seed argument loop in internal_test_main.cpp with CLI11 (shared/CLI11.hpp), as used by rocfft-test and the other rocFFT tools. Accepts --seed <value> and --seed=<value>, rejects unknown arguments, and adds --help.
Contributor
TheRock Submodule Bump ActivityNewest first
|
This was referenced Oct 27, 2026
regan-amd
added a commit
that referenced
this pull request
Nov 2, 2026
#14505) ## Motivation Follow-up to #12667. The optional dimension-ordering argument of `default_strides`, `default_distances` and `default_distance` has a half-baked contract (e.g., silently invalid strides for real in-place transforms whose last dimension is not contiguous), and it only ever served one consumer: `setup_nondefault_dim_ordering_plan` in `hipfftw_test.cpp`. That unclear contract invited misuse and confusion about its purpose, both in #12667 and earlier in #5761. Rather than patching it, this PR simplifies the helpers' contract altogether and moves the consumer-specific logic into the one test that needs it. JIRA ID: AIFFT-543 ## Technical Details - `client_data_layout_helpers.h`: remove `dim_order` / `len_dim_order` from `default_strides`, `default_distances` and `default_distance`, along with the related validation and docs. The rocFFT and hipFFT copies remain identical. All other callers only used the default arguments, for which the behavior is unchanged. - `hipfftw_test.cpp` (`setup_nondefault_dim_ordering_plan`): compute the packed data extents per side (Hermitian-reduced last dimension, plus padding on the real side of in-place transforms), then derive strides and distances with a local `packed_layout` lambda from the shuffled dimension ordering. - Test coverage is unchanged: for a given seed, the generated test cases are identical. Random draws happen in the same order, and the strides and distances match the previous values. ## Test Plan - Locally checked that the new strides and distances in `setup_nondefault_dim_ordering_plan` match the previous helper-based values exactly, on 20,000 random configurations generated the same way as in the test (all transform types and placements, ranks 2-3). - Compiled the hipFFT library sources that include the header (`hipfft.cpp`, `hipfftw.cpp`) and `hipfftw_test.cpp`. - Run `hipfftw-test` and the regular hipFFT and rocFFT client test suites in CI. ## Test Result - Local equivalence check: 0 mismatches out of 20,000. - Local compilation: OK. - CI: pending. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Abuudiii
added a commit
that referenced
this pull request
Nov 3, 2026
## Motivation JIRA ID : AIFFT-561 Renames `data_layout_t` to `data_set_t` before the struct is extended to describe a full data set in a buffer, not only its layout. No functional change. Depends on #12667, which adds the test file renamed here. ## Technical Details - `data_layout.{h,cpp}` -> `data_set.{h,cpp}`, `data_layout_test.cpp` -> `data_set_test.cpp`, CMake lists updated. The name avoids confusion with the existing `data_descriptor.h` (solution-map serializer). - Members that refer to a layout are renamed: `full_layout` -> `full_range`, `default_full_layout` -> `default_full_range`, `get_layout_for_len_axes` -> `get_data_set_for_len_axes`, `get_embedding_layout` -> `get_embedding_data_set`, `get_other_inplace_layout_for` -> `get_other_inplace_data_set_for`. - The struct's documentation states that it describes in-buffer data without holding any. - `plan.{h,cpp}`, `tree_node.{h,cpp}`: uses of the type and members renamed. - Members outside the struct (`rocfft_brick_t::layout`, `input_layout`/`output_layout`, `undistributed_layout_for`) are left for a separate mechanical PR at the end of the stack. Public `rocfft_plan_description_set_data_layout` is unchanged. ## Test Plan Built `rocfft` and `rocfft-internal-test` on Navi4x (gfx1201), ROCm 7.2.4, and ran every `rocfft-internal-test` case in its own process. ## Test Result 19/19 pass, including the 15 `data_set_test` cases. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Split from #11272.
data_layoutstride and offset arithmetic had no direct tests.Technical Details
data_layout_test.cpp- constructor validation, packed default strides, offsets, in-place real/complex layouts and axis orderingshared/ptrdiff.h- adds the missing#include <algorithm>forstd::any_of; it did not build without it once the test compileddata_layout.cppTest Plan
Built this branch alone on develop and ran every
rocfft-internal-testcase in its own process on MI210 (gfx90a), ROCm 7.1.0.Test Result
15/15 new tests pass, 19/19 total.
Submission Checklist