fix(gather_v2/view_copy): convert element offsets to byte displacements for MGATHER/MSCATTER - #134
wangyuascend-spec wants to merge 1 commit into
Conversation
…ts for MGATHER/MSCATTER MGATHER/MSCATTER take byte displacements from the base pointer (PTO v0.58.4: "each element is a signed/unsigned byte displacement. Do not scale by the transfer element width" — confirmed from gfrun ExecuteMGATHER/ExecuteMSCATTER). Both kernels were accumulating offsets in element units and passing them directly, silently reading/writing at 1/sizeof(DType) of the intended position for fp16/fp32 data. gather_v2: scale index_offset by sizeof(IType) before the index-tile MGATHER, and scale the accumulated input_offset_tile by sizeof(DType) before the final data MGATHER. view_copy: scale both input_offset_tile and output_offset_tile by sizeof(DType) before MGATHER/MSCATTER. Validation (gfrun 4PE, embedded oracle, R2): - gather_v2 rank1 fp32, rank2 half, rank2 int32: R2=1→0 (all PASS) - view_copy half×2, int32: R2=1→0 (all PASS)
![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: 8ca0815bfb
ℹ️ 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".
| TMULS(input_offset_tile, input_offset_tile, | ||
| static_cast<std::uint32_t>(sizeof(DType))); |
There was a problem hiding this comment.
When the input occupies more than 4 GiB, this multiplication wraps because input_offset_tile is a uint32_t tile. For example, a rank-1 float gather with 1,073,741,825 input elements maps the final element's displacement to zero, silently reading the first element instead. The API currently permits that InputElements value and documents no size restriction, so add a representability check or use a wider offset tile; the index-buffer scaling above needs the equivalent protection.
Useful? React with 👍 / 👎.
1 participant
Summary
MGATHER/MSCATTER take byte displacements from the base pointer (PTO v0.58.4: "each element is a signed/unsigned byte displacement. Do not scale by the transfer element width" — confirmed from gfrun
ExecuteMGATHER/ExecuteMSCATTER). Both kernels were accumulating offsets in element units and passing them directly, silently reading/writing at1/sizeof(DType)of the intended position for fp16/fp32 data.index_offsetbysizeof(IType)before the index-tile MGATHER, and scale the accumulatedinput_offset_tilebysizeof(DType)before the final data MGATHER.input_offset_tileandoutput_offset_tilebysizeof(DType)before MGATHER/MSCATTER.Evidence that offsets are bytes (not elements):
ExecuteMGATHERcomment: "byte displacement. Do not scale by the transfer element width"hashtable_lookup_simd.hpp:175,microbenchmark mgather_fp32) all multiply bysizeofValidation
Base:
main@64d2d77(includes merged PR #133)gfrun 4PE, embedded oracle (host reference + tolerance in main), rc=0,
Reach the End of Benchmarkfor all runs.