Repository navigation
refactor(rocfft): remove is_partial flag and validate fields at construction - #8718
Merged
regan-amd merged 7 commits intoOct 2, 2026
Merged
Conversation
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (47.97%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #8718 +/- ##
===========================================
+ Coverage 70.23% 70.28% +0.06%
===========================================
Files 2810 2810
Lines 462535 462582 +47
Branches 68103 68105 +2
===========================================
+ Hits 324833 325123 +290
+ Misses 114182 113886 -296
- Partials 23520 23573 +53
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
regan-amd
force-pushed
the
users/regan-amd/no_partial_flag_in_data_layout
branch
3 times, most recently
from
June 23, 2026 18:40
8ab70b8 to
10482e9
Compare
regan-amd
marked this pull request as ready for review
June 23, 2026 19:58
Contributor
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
Contributor
Author
Other priorities have taken precedence. I will get back to this work hopefully soon! |
Contributor
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
github-actions
Bot
added
Stale
regan-amd
changed the title
is_partial flag from data_layout_t and make rocfft_field_t validate bricks at constructionis_partial flag and validate fields at construction
regan-amd
force-pushed
the
users/regan-amd/no_partial_flag_in_data_layout
branch
from
September 8, 2026 22:55
10482e9 to
fa93634
Compare
✅ 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. |
Add an overload accepting a vector of batch sizes for multi-batch layouts, and keep a convenience single-batch inline wrapper for backward compatibility.
Make bricks private with accessor methods (get_bricks, get_brick, num_bricks, empty). Compute global_data_range and undistributed_length_dimensions eagerly in a validating constructor that checks tessellation. Add field-level erase_unit_length_axis() and reorder_length_axes() operations.
Remove the is_partial member and all code that sets/reads it. Replace has_some_partial_length_axis() with direct lower-bound checks where needed. Change accessor exception type to std::out_of_range for semantic correctness.
Let callers choose to move (transfer ownership) or copy (keep their own vector for reuse) rather than mandating ownership transfer via rvalue ref.
… transpose The RCCL-guarded GlobalTransposeRCCL and InitRCCLCommunicator code accessed the private bricks member of rocfft_field_t directly, which failed to compile with ROCFFT_RCCL_ENABLE=ON. Switch to the public num_bricks()/get_brick()/ get_bricks() accessors.
regan-amd
force-pushed
the
users/regan-amd/no_partial_flag_in_data_layout
branch
from
September 14, 2026 17:55
dcc9166 to
cc0607f
Compare
evetsso
approved these changes
Sep 16, 2026
malcolmroberts
approved these changes
Oct 2, 2026
1 task done
ScottTodd
pushed a commit
that referenced
this pull request
Oct 2, 2026
…icks (#12967) ## Motivation #8718 made `rocfft_field_t::bricks` private, breaking the contiguity check in `plan.cpp`. ## Technical Details Self-explanatory ## Test Plan Re-build and test. ## Test Result Build succeeds. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.
Contributor
TheRock Submodule Bump ActivityNewest first
|
1 task done
1 task done
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 (edited 09/08/2026)
The
is_partialflag ondata_layout_t::axis_tis context-dependent: it's relevant only for layouts attached to field's bricks, it's irrelevant otherwise (supposed and expected to befalse) for any non-distributed data, e.g., as for any single-device usage.Being context-dependent, any modification to the context, i.e., to a field, must trigger a modification to all the bricks' layouts. This is generating unnecessary maintenance costs, and prone to let internally-inconsistent field objects be created.
This PR removes the
is_partialflag fromdata_layout_t::axis_tand tighten up howrocfft_field_tobjects (i.e., the context) may be created and/or modified to prevent the creation of internally-inconsistent fields by design.These changes are also motivated by possible existing bottlenecks for distributed workloads involving fields with large numbers of bricks: caching derived state (global data range, undistributed dimensions) at construction avoids redundant re-computation on every query. The field's constructor takes its bricks by value, so callers can move them in to transfer ownership or pass an lvalue to keep their own copy.
JIRA ID: AIFFT-475
Technical Details (edited 09/08/2026)
is_partialmember fromdata_layout_t::axis_tand all code that sets or reads it (has_some_partial_length_axis(), the flag inmake_contiguous_intersection_of, etc.);rocfft_field_t::bricksprivate; expose read-only access viaget_bricks(),get_brick(i),num_bricks(), andempty();rocfft_field_t's constructor, making invalid fields unrepresentable (fail-fast on construction rather than relying on separate finalize() + has_valid_tessellation() calls);get_global_data_range()andget_undistributed_length_dimensions();erase_unit_length_axis()andreorder_length_axes()private member functions torocfft_field_tthat maintain internal invariants (global range, undistributed dimensions set) consistently;data_layout_t::default_full_layoutto accept multi-dimensional batches (std::vector<size_t>) alongside a convenience overload for the common single-batch case.rocfft_field_t's constructor take its bricks by value (std::vector<rocfft_brick_t>) so ownership transfer is the caller's choice (move in to transfer, pass an lvalue to keep a copy) rather than being mandated via an rvalue reference.Test Plan
The current multi-device integration tests cover all the suggested changes.
Test Result
Tests pass on 2 or 8
gfx90adevices, using tests for single- or multi-process usage.Submission Checklist