[CK] Refactor container array and function sequence_reverse_inclusive_scan#3612
[CK] Refactor container array and function sequence_reverse_inclusive_scan#3612
Conversation
include/ck/utility/array.hpp
Outdated
| #pragma unroll | ||
| for(int i = 0; i < NSize; i++) | ||
| { | ||
| mData[i] = a[i]; | ||
| } |
There was a problem hiding this comment.
Removed the dependency on static_for. Hence, only need to include type.hpp.
And the code is easier to read.
There was a problem hiding this comment.
Pull request overview
This PR refactors the sequence_reverse_inclusive_scan implementation from a recursive template-based approach to a more efficient constexpr for-loop using the Array utility class. The changes improve compile-time performance and simplify the logic.
Changes:
- Refactored
sequence_reverse_inclusive_scanto use an array-based iterative approach instead of recursive templates - Updated header dependencies to remove circular includes and optimize dependencies
- Optimized
Array::operator=by replacingstatic_forwith a standard for-loop with unroll pragma - Added comprehensive unit tests for the
Arrayclass
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| include/ck/utility/sequence.hpp | Refactored sequence_reverse_inclusive_scan from recursive templates to constexpr array-based implementation |
| include/ck/utility/array.hpp | Updated header dependencies and optimized assignment operator with standard for-loop |
| test/util/unit_array.cpp | Added comprehensive unit tests covering Array construction, access, assignment, iterators, and edge cases |
| test/util/CMakeLists.txt | Added build configuration for new unit_array test executable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/util/unit_array.cpp
Outdated
|
|
||
| using namespace ck; | ||
|
|
||
| // Test basic Sequence construction and properties |
There was a problem hiding this comment.
Comment mentions 'Sequence' but should refer to 'Array' since this test is for the Array class.
| // Test basic Sequence construction and properties | |
| // Test basic Array construction and properties |
There was a problem hiding this comment.
This comment should be updated.
include/ck/utility/array.hpp
Outdated
|
|
||
| static_for<0, Size(), 1>{}([&](auto i) { operator()(i) = a[i]; }); | ||
| #pragma unroll | ||
| for(int i = 0; i < NSize; i++) |
There was a problem hiding this comment.
The loop variable uses int type while NSize is of type index_t (typically size_t or similar). For consistency and to avoid potential type conversion issues, use index_t i = 0 instead of int i = 0.
| for(int i = 0; i < NSize; i++) | |
| for(index_t i = 0; i < NSize; i++) |
There was a problem hiding this comment.
Yes, let's use index_t for consistency.
include/ck/utility/array.hpp
Outdated
|
|
||
| static_for<0, Size(), 1>{}([&](auto i) { operator()(i) = a[i]; }); | ||
| #pragma unroll | ||
| for(int i = 0; i < NSize; i++) |
There was a problem hiding this comment.
We don't need pragma unroll here. The compiler should optimize correctly.
include/ck/utility/sequence.hpp
Outdated
| } | ||
| else | ||
| { | ||
| constexpr Array<index_t, size> arr = []() { |
There was a problem hiding this comment.
It might be slightly faster to replace this lambda with a named constexpr function helper.
shumway
left a comment
There was a problem hiding this comment.
This looks great. I just have a few small comments.
I think we may need unit tests for the reverse inclusive scan.
|
Hey @CongMa13, please resolve conflicts for the headers, and make sure ci tests pass. |
bf61492 to
0995ef9
Compare
0995ef9 to
897f8d2
Compare
897f8d2 to
add4100
Compare
|
Imported to ROCm/rocm-libraries |
Refactor cd
sequence_reverse_inclusive_scanfrom recursive template to constexpr for-loop.This pull request improves the utility array functionality and its integration with sequence utilities, as well as adds comprehensive unit tests for the
Arrayclass. The main changes include refactoring the implementation of reverse inclusive scan for sequences, updating header dependencies, optimizing theArrayassignment operator, and introducing a full test suite forArray.Array and Sequence Utility Improvements:
sequence_reverse_inclusive_scaninsequence.hppto use a more efficient, array-based approach, encapsulated in a newimplnamespace. This change simplifies the logic and should improve compile-time performance.array.hppandsequence.hppto remove unused includes and ensure correct dependency order, notably includingarray.hppinsequence.hppto support the new scan implementation. [1] [2]Arraystruct by replacing thestatic_forloop with a standard for-loop and adding a pragma for potential unrolling, improving both readability and performance.Testing Enhancements:
unit_array.cppwith comprehensive tests covering construction, element access, assignment, iterators, themake_arrayhelper, and different data types for theArrayclass.CMakeLists.txtto build and link the newunit_arraytest executable.Tracking issue: #3575