fix(grouped_gemm): initialize Xe2 scheduler counter before launch - #524
fix(grouped_gemm): initialize Xe2 scheduler counter before launch#524SergiioB wants to merge 1 commit into
Conversation
| // starting tile index (especially under XPU graph replay). | ||
| at::Tensor atomic_buffer = | ||
| at::empty({static_cast<long>(1)}, ptr_A.options().dtype(at::kInt)); | ||
| at::zeros({static_cast<long>(1)}, ptr_A.options().dtype(at::kInt)); |
There was a problem hiding this comment.
If we change empty to zero here, please remove the zero-initialization operation in the subsequent kernel.
There was a problem hiding this comment.
And note: This change is reasonable, but it may result in a slight performance degradation.
|
we may need more evaluation about graph behavior. |
The scheduler counter is allocated with at::empty. The kernel assumes group 0 stores 0 before other groups atomicAdd, but SYCL does not guarantee that group 0 runs first. A leftover integer becomes the starting tile index; XPU graph replay makes this systematic because the captured buffer address is reused. at::zeros queues fill_(0) happens-before the GEMM and is part of the captured graph, so every replay starts at 0. This is scheduler-counter init only; the GEMM reduction itself is tile-disjoint. Signed-off-by: SergiioB <sergiobarrientose@outlook.com>
d34e28a to
4043e76
Compare
|
Addressed the review comments in the rewritten branch:
The host-side initialization provides launch ordering. The removed in-kernel relaxed store had no device-wide barrier before other work-groups reached I agree that |
|
Current status after rechecking the full thread: all recorded BMG/PVC build and unit-test jobs are green, and the paged-decode change was correctly split into merged #526. The remaining review request is still the graph-replay correctness and matched small-M latency evaluation for the host-side I have not produced that measurement yet, so I am not claiming the PR is ready based on CI alone. Keeping the PR open pending that evidence or maintainer direction on whether the fixed dispatch cost is acceptable. |
Purpose
Initialize the Xe2 grouped-GEMM persistent scheduler counter before launching the kernel, and remove the now-redundant in-kernel zero store.
The previous initialization was performed only by work-group 0, local thread 0. There is no device-wide barrier before other work-groups execute
atomicAdd, so another work-group can observe stale state, or a late store can reset a counter that has already been incremented.Using
at::zerosqueues initialization before the grouped-GEMM launch. The in-kernelatomic_ref::store(0)is removed as requested in review. The local barrier after each work-group obtains a scheduler ticket remains unchanged.The paged-decode tuple originally included in this PR was cherry-picked and merged separately in #526. This PR is now rebased onto that commit and contains only the grouped-GEMM fix.
Performance consideration
Although the buffer is one
int32,at::zerosmay add a fixed dispatch/dependency cost, most visible for small grouped-GEMM calls. Removing the in-kernel store avoids paying both initialization paths. This PR does not claim zero overhead without a matched measurement.Test plan
Current validation status
main/ merged feat(attn): add paged-decode tuple 16,128,64,false,true,false #526.paged_decode_default.confhas no PR diff.git diff --check: pass.