[STF] Refactor data_place to use polymorphic design#7976
Open
andralex wants to merge 10 commits intoNVIDIA:mainfrom
Open
[STF] Refactor data_place to use polymorphic design#7976andralex wants to merge 10 commits intoNVIDIA:mainfrom
andralex wants to merge 10 commits intoNVIDIA:mainfrom
Conversation
Replace the ad-hoc tagged union design in data_place with a clean polymorphic architecture using data_place_interface as the abstract base class. Key changes: - Add data_place_interface.cuh defining the virtual interface for all data place types with sensible defaults for hash/equals/less_than - Add data_place_impl.cuh with concrete implementations: data_place_host, data_place_managed, data_place_device, data_place_affine, etc. - Refactor data_place to hold shared_ptr<data_place_interface> and delegate all operations to the implementation - Use static instances with no-op deleters for common singletons (host, managed, invalid) to avoid repeated allocations - Remove data_place_extension.cuh - extensions now inherit directly from data_place_interface and provide get_affine_exec_impl() - Update green_context to inherit from data_place_interface directly This design enables cleaner extensibility and eliminates the awkward mix of enum-based type tags with extension pointers. Made-with: Cursor
Contributor
Contributor
Author
|
pre-commit.ci autofix |
2 tasks
Require each concrete data_place implementation to provide its own hash() instead of relying on the default get_device_ordinal()-based implementation. Also fix stray includes of the getStream/getDataStream rename and a duplicate interpreted_execution_policy constructor that shouldn't have been in this PR. Made-with: Cursor
- Make hash() pure virtual; each concrete class provides its own implementation - Replace equals()/less_than() with a single pure virtual cmp() returning -1/0/1; data_place::operator==/< delegate to pimpl_->cmp() directly - Move device ordinals into data_place_interface as a nested unscoped enum ord, accessible as data_place_interface::host, ::managed, etc. - Cache data_place_device instances in a static array (same pattern as exec_place::device), avoiding per-call allocations - Remove nop_deleter struct; inline the lambda directly in make_static_instance - data_place::device() now asserts dev_id >= 0, dropping legacy negative-id special cases Made-with: Cursor
Contributor
Author
|
pre-commit.ci autofix |
Contributor
Author
|
/ok to test d67f93f |
Contributor
Author
|
/ok to test 10c3877 |
This comment has been minimized.
This comment has been minimized.
…posite::cmp() Replace ordered comparison of function pointers with std::less, which provides a well-defined total order on pointers without triggering Clang's -Wordered-compare-function-pointers diagnostic. Made-with: Cursor
Contributor
Author
|
/ok to test 4d74379 |
This comment has been minimized.
This comment has been minimized.
caugonnet
reviewed
Mar 10, 2026
|
|
||
| // For simple places, compare devid | ||
| return devid < rhs.devid; | ||
| EXPECT((!is_composite() && !rhs.is_composite()), "Ordering of composite places is not implemented."); |
…conversion Use structural grid comparison (shape + element-by-element) in data_place_composite::cmp(), matching the exec_place_grid comparison model as Cedric suggested. Add from_index() as the proper inverse of to_index(), and fix the reduction plan code that was incorrectly using data_place::device(n-2) for all node indices, which crashed when n < 2 (host/managed nodes). Made-with: Cursor
Contributor
Author
|
/ok to test |
Contributor
@andralex, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
Contributor
Author
|
/ok to test 536826a |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
/ok to test 3542521 |
The friend definition inside data_place is only found via ADL when a data_place argument is present. Calls like from_index(n) in logical_data.cuh have no data_place argument, so add a matching declaration at namespace scope so the identifier is visible to unqualified lookup. Made-with: Cursor
andralex
force-pushed
the
stf_data_place_polymorphic
branch
from
3542521 to
ede252b
Compare
Contributor
Author
|
/ok to test ede252b |
Contributor
😬 CI Workflow Results🟥 Finished in 31m 33s: Pass: 93%/48 | Total: 12h 15m | Max: 25m 03s | Hits: 68%/21910See results here. |
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
2 participants
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.
Summary
Replaces the ad-hoc tagged union design in
data_placewith a clean polymorphic architecture.Before:
data_placeused anint devidplus separateshared_ptr<composite_state>andshared_ptr<data_place_extension>members — an awkward mix of enum-based type tags with extension pointers.After:
data_place_interface(newdata_place_interface.cuh) — abstract virtual interface for all place types, with sensible defaults forhash(),equals(),less_than(), andmem_create()data_place_impl.cuh— concrete implementations:data_place_host,data_place_managed,data_place_device,data_place_affine,data_place_device_auto,data_place_invaliddata_place_composite— defined inplaces.cuhafterexec_place_grid(needed due to its dependency on that type)data_place— now holds ashared_ptr<data_place_interface>and delegates all operations to itdata_place_extension.cuh— custom extensions now inherit directly fromdata_place_interfaceTest plan
cudax.test.stfand verify no regressionsMade with Cursor