add view_copy and gather_v2 - #79
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6359467023
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| IndexGlobal index_global(index); | ||
| OutputGlobal output_global(output); | ||
|
|
||
| constexpr std::uint32_t ThreadNum = 4; |
There was a problem hiding this comment.
When this kernel is launched with anything other than exactly four PEs, partitioning by the hard-coded ThreadNum produces incorrect output: fewer PEs leave tiles assigned to absent workers untouched (including tails whose owner is PE1–PE3), while additional PEs duplicate work assigned to PE0–PE3. This also contradicts the function comment promising a single-thread default and a caller-selected launch count, which the template currently cannot accept.
Useful? React with 👍 / 👎.
| InputGlobal input_global(input + input_offset / sizeof(DType)); | ||
| OutputGlobal output_global(output + output_offset / sizeof(DType)); | ||
|
|
||
| constexpr std::uint32_t ThreadNum = 4; |
There was a problem hiding this comment.
When tile_view_copy runs with fewer than four PEs, only every fourth tile beginning at each available thread ID is processed, and a tail owned by a missing PE is skipped; with more than four PEs, later workers overlap earlier workers' tiles. The API comment says the regular single-thread case should default to one and multi-thread callers should supply their launch count, but no such template parameter exists.
Useful? React with 👍 / 👎.
| TLOAD(index_tile, tile_index_global); | ||
| MGATHER<pto::Coalesce::Row>(output_tile, table_global, index_tile); | ||
| TSTORE(tile_output_global, output_tile); |
There was a problem hiding this comment.
For the rank-2 axis-0 fast path with exact-size caller buffers, these operations use a 128-byte physical index tile and a TileElements-wide physical data tile even though only one index and row_width data elements are valid. The added test explicitly pads all three backing buffers to keep the final physical transaction within storage (test/kernel/gather_v2/src/gather_v2.cpp lines 74–82), but the public kernel contract does not require that padding, so the last index or row can make a transaction cross the allocation or a page boundary.
Useful? React with 👍 / 👎.
| #!/bin/bash | ||
| # view_copy operator compile cases |
There was a problem hiding this comment.
The documented whole-backend build still invokes only the pre-existing operator directories: inspection of benchmark/one-level-arch/compile_all.sh shows no compile_operator call for either view_copy or gather_v2. Consequently, running that batch silently skips both newly added compile suites, so these kernels receive no coverage through the repository-wide compilation workflow.
Useful? React with 👍 / 👎.
| alignas(32) dtype input[INPUT_ELEMENTSs + kDataPaddingElements] = {}; | ||
| alignas(32) itype index[INDEX_ELEMENTSs + kIndexPaddingElements] = {}; | ||
| alignas(32) dtype output[OUTPUT_ELEMENTSs + kDataPaddingElements]; |
There was a problem hiding this comment.
When either new harness is executed on the Linx target, its global-memory arrays are only 32-byte aligned, whereas the repository's PTO programming guide requires 4 KB alignment for DMA tile operations and existing executable tests use the ALIGN_MASK + ALIGN allocation pattern. The gather harness reaches TLOAD/TSTORE, and the view-copy harness likewise passes its 32-byte-aligned arrays to TLSU memory operations, so runtime testing can fail for alignment reasons before it validates either kernel.
Useful? React with 👍 / 👎.
| using IndexTile = | ||
| pto::Tile<pto::Location::Vec, IType, 1, TileElements, | ||
| pto::BLayout::RowMajor, 1, ValidElements>; |
There was a problem hiding this comment.
When the generic path is instantiated with a narrow index type, the current checks can all pass while IndexTile violates the hardware's 128-byte minimum tile size—for example, DType=float, IType=uint16_t, and TileElements=32 produce a 64-byte index tile. The data and offset tiles have explicit minimum-size checks, but this third physical tile does not, so otherwise valid template arguments can generate an unrepresentable tile instead of being rejected or using a padded index-tile width.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3af977328
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #include <cstdint> | ||
| #include <cstdio> | ||
|
|
||
| #include "view_copy/view_copy_pto.hpp" |
There was a problem hiding this comment.
When any view_copy/compile.all case is built, preprocessing fails because this include names view_copy_pto.hpp, while a repo-wide tree search at this commit finds only kernels/view_copy/view_copy.hpp. Update the include or rename the header so the new test suite can compile.
Useful? React with 👍 / 👎.
| TMULS(contribution, coordinate, | ||
| input_stride * static_cast<std::uint32_t>(sizeof(DType))); |
There was a problem hiding this comment.
When a valid input occupies more than 4 GiB (for example, a sufficiently large float tensor), input_stride * sizeof(DType) and the accumulated MGATHER byte offsets wrap modulo 2^32, so indices into later portions of the tensor read unrelated earlier elements. The public contract permits this because it only requires the shape product to equal InputElements; add the same byte-span limit used by the transpose kernel or otherwise handle wider offsets.
Useful? React with 👍 / 👎.
1 participant
No description provided.