Repository navigation
[Bugfix][Triton] Fix int32 KV-offset overflow in _mla_gluon >2GB path - #4474
Merged
Merged
Conversation
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
dllehr-amd
previously approved these changes
Jul 31, 2026
dllehr-amd
left a comment
Contributor
There was a problem hiding this comment.
Thanks @peizhang56 ! Looks good on my side!
On the WITHIN_2GB=False path the KV offset kv_loc * stride_kv_c_bs is computed in int32 and overflows (that overflow is precisely why this branch exists), yielding a wild 64-bit pointer in the global_load fallback and a SIGSEGV in _mla_gluon. Promote stride_kv_c_bs and stride_k_pe_bs to int64 when not WITHIN_2GB so the whole offset computes in 64-bit. The buffer_load path (WITHIN_2GB=True) keeps 32-bit offsets by hardware requirement; the guard is constexpr so it folds out there. Signed-off-by: pei.zhang <pei.zhang@amd.com>
peizhang56
force-pushed
the
fix_k3_prefix_cache_spec_decode
branch
from
July 31, 2026 21:40
95e6195 to
1e4135b
Compare
JohnQin-llm
added a commit
to JohnQin-llm/aiter-amd
that referenced
this pull request
Aug 2, 2026
KV addresses are built as kv_loc * stride_kv_c_bs + offs_d with both operands
int32, so for a 576-wide bf16 row the product wraps once a page id passes
2**31 // 576 = 3,728,270 and the load lands below the cache base:
Memory access fault by GPU node-N ... Reason: Unknown
The existing cases in test_mla.py already compile and run the WITHIN_2GB=False
branch -- kv_max_sz = 65536*32 = 2,097,152 rows is 2.25 GiB, over the 2 GB
cutoff -- but page ids top out at 2,097,151, a factor of 1.78 below the wrap
point, so the branch cannot trigger the overflow. --sequential_page_indices,
which is commented "byte offset can exceed 2^32", reaches ~2,107,152 rows at
batch 256 x ctx 8192 and is still 1.77x short.
Add a case with a 4.72 GiB cache (4,400,000 rows) indexed at row 4,255,488,
covering both a BLOCK_N-aligned length and a partial trailing tile.
The launch runs in a subprocess: a GPU memory fault aborts the interpreter
rather than raising, so an in-process test would SIGABRT and take out the
remaining files in the CI shard instead of reporting one failure. Verified in
both directions on 8x MI355X -- with the fix the child exits 0, on unfixed main
the child exits -6 and the parent reports a clean AssertionError and continues.
Gated on gfx950 and on torch.cuda.mem_get_info() with a 1.3x margin, following
the convention in test_batch_prefill.py.
Depends on ROCm#4474: this test fails on main until that fix merges.
Signed-off-by: Yanyuan Qin <yanyuan.qin@amd.com>
Contributor
Author
|
Hi @valarLip will you be able to help to review this code change. |
Open
1 task done
valarLip
approved these changes
Aug 4, 2026
3 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On the WITHIN_2GB=False path the KV offset kv_loc * stride_kv_c_bs is computed in int32 and overflows (that overflow is precisely why this branch exists), yielding a wild 64-bit pointer in the global_load fallback and a SIGSEGV in _mla_gluon.
Promote stride_kv_c_bs and stride_k_pe_bs to int64 when not WITHIN_2GB so the whole offset computes in 64-bit. The buffer_load path (WITHIN_2GB=True) keeps 32-bit offsets by hardware requirement; the guard is constexpr so it folds out there.
Motivation
_mla_gluoncrashes with a SIGSEGV when the KV cache exceeds 2GB. In that case the kernel selects theWITHIN_2GB=Falsecode path, but the KV offsetkv_loc * stride_kv_c_bsis still computed in int32. For a >2GB cache this multiplication overflows, and the overflowed offset is added to the 64-bit base pointer in theglobal_loadfallback, producing an out-of-bounds pointer and a segfault. The overflow is precisely the condition that selects this branch, so the offset must be computed in 64-bit.Technical Details
Promote
stride_kv_c_bsandstride_k_pe_bstogl.int64whennot WITHIN_2GB, so every downstream offset computation (kv_loc * stride_kv_c_bs + ...for the KV cache, and the correspondingstride_k_pe_bsexpressions for K_pe) is evaluated in 64-bit and no longer overflows. The reassignment happens before the first offset is built, so all load sites pick up the widened strides. Thebuffer_loadpath (WITHIN_2GB=True) intentionally keeps 32-bit offsets, which is a hardware requirement ofbuffer_load_to_shared; sinceWITHIN_2GBis agl.constexpr, the guard folds out entirely on that path and there is no runtime or performance impact.Test Plan
Ran the MLA gluon kernel with a KV cache sized above 2GB to force the
WITHIN_2GB=False/global_loadpath, and confirmed the same configuration crashed before the change. Also exercised theWITHIN_2GB=Truepath to confirm no behavioral or performance change (the guard folds out at compile time).Test Result
Before: SIGSEGV in
_mla_gluonon the >2GB KV-cache configuration. After: the kernel runs correctly and produces expected output; theWITHIN_2GB=Truepath is unchanged. No new unit test is added because reproducing this reliably requires allocating a >2GB KV cache, which is impractical in the standard op_test harness.Submission Checklist