-
Notifications
You must be signed in to change notification settings - Fork 269
[CK TILE] Refactor sequence_reverse_inclusive_scan #3609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Thanks! Could you also measure the build metrics improvements, e.g. number of templates instantiated, or wall time improvement for some target? |
| template <index_t I, index_t... Is, typename Reduce, index_t Init> | ||
| struct sequence_reverse_inclusive_scan<sequence<I, Is...>, Reduce, Init> | ||
| template <index_t... Is, typename Reduce, index_t Init> | ||
| struct sequence_reverse_inclusive_scan_impl<sequence<Is...>, Reduce, Init> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: implementation, would it help to have separate compile-time functions for sequence reversal (constexpr function with c-array initialized with variadic indices and then reverse using a for-loop logic) and scan (with a similar structure)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor the code and added a reverse function
- Add trivial_array for template programming - Add unit tests of sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the sequence_reverse_inclusive_scan implementation in the CK TILE library from a recursive template-based approach to an iterative for-loop implementation. The changes improve compilation efficiency by reducing template instantiation depth.
Changes:
- Refactored
sequence_reverse_inclusive_scanfrom recursive to iterative implementation usingtrivial_array - Added comprehensive unit tests for sequence operations
- Introduced
trivial_arraycontainer for constexpr operations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| include/ck_tile/core/container/sequence.hpp | Refactored sequence_reverse_inclusive_scan to use iterative approach with trivial_array, replaced array with trivial_array in utility functions |
| include/ck_tile/core/container/trivial_array.hpp | New file introducing trivial_array container for constexpr aggregate initialization |
| include/ck_tile/core.hpp | Added include for new trivial_array.hpp header |
| test/ck_tile/core/container/unit_sequence.cpp | New comprehensive test suite covering sequence operations including scan, transform, reduce, and utility functions |
| test/ck_tile/core/container/CMakeLists.txt | Build configuration for new sequence unit tests |
| test/ck_tile/core/CMakeLists.txt | Added container subdirectory to test build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(result EQUAL 0) | ||
| target_link_libraries(ck_tile_unit_sequence PRIVATE utility) | ||
| endif() |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable result is referenced but never defined. This condition will always fail. The add_gtest_executable macro likely needs to set a return value variable, or this check should reference a variable that the macro actually sets.
| if(result EQUAL 0) | |
| target_link_libraries(ck_tile_unit_sequence PRIVATE utility) | |
| endif() | |
| target_link_libraries(ck_tile_unit_sequence PRIVATE utility) |
Proposed changes
Refactor ck tile
sequence_reverse_inclusive_scanfrom recursive to for-loop.Tracking issue: #3575
This pull request introduces a new lightweight array type,
trivial_array, and refactors the sequence utilities to use it for improved constexpr support and simplicity. The changes also include updates to the build system to add container-related tests.Core Library Improvements:
trivial_array.hppthat defines thetrivial_arraytype, a constexpr-friendly array with basic accessors and no custom constructors.core.hppandsequence.hppto importtrivial_array. [1] [2]Refactoring to Use
trivial_array:sequence.hppto usetrivial_arrayinstead of the previously forward-declaredarraytype, including in histogram and array generation logic. [1] [2]sequence_reverse_inclusive_scanto usetrivial_arrayfor intermediate storage, improving constexpr evaluation and clarity.Build System and Testing:
unit_sequence.cppto the CMake build configuration. [1] [2]