[ROCm] Widen the HiCache JIT copy rounds and enable the K-only host pool - #37152
Conversation
|
Could a maintainer add the What I ran locally in the meantime, on MI300X:
The 46 new cases in this PR are all CPU-only and land in the |
|
/tag-and-rerun-ci |
|
@AMD-yanfeiwang Parts of this change overlap with #35233 |
e7d0ba5 to
d5c2ba9
Compare
|
/rerun-failed-ci |
4018ca5 to
aabde70
Compare
@Duyi-Wang You're right, and I've rescoped this. The version you commented on carried a pointer-domain fix of its own: allocating ROCm host pools with #35233 is the better fix for the part that overlapped. Translating to the device alias at the launch boundary is more general than changing how we allocate: it covers registered buffers as well as One caveat on that, which I've raised on #35233 rather than here: the alias's offset arithmetic is only valid within a single registration, and On the two approaches performing about the same: for the default allocator, agreed, and I have nothing that separates them. For the storage-owned case they aren't equivalent, because the downgrade meant running Two places the PRs still touch:
|
|
@HaiShaw add accuracy reading and performance reading in 3 way, for baseline, baseline + #35233, and baseline + #35233 +#37152 baseline docker image: Accuracy. GSM8K 200-shot (5-shot, real MTP) is in-band across arms:
Speed. TP4/EP4 GLM-5.2-MXFP4, 8×MI355X, HiCache on. Same recipe besides HiCache backend:
c14 also: GPU cache hit ~80% on all three; per-GPU total ~11.5k ( Reading. |
Widen the transfer kernel's copy round from a fixed 128 B to the widest of 128/64/32/16 B that tiles the element size across the worker's lanes, so sizes 128 does not divide -- MLA's 576 B fp8 row -- can use the JIT at all. The narrow rounds only pay off against a grid that stays fed, so they and the raised block quota are both gated to ROCm; CUDA keeps the 128 B requirement and quota 2. Co-authored-by: Cursor <cursoragent@cursor.com>
MHATokenToKVPoolHost's gates already admit HIP; this index-K pool was left on _is_cuda with no stated reason. Nothing about it is CUDA-specific, and can_use_hicache_jit_kernel() already returns True on ROCm for both of its element sizes. Depends on the pin_memory allocation: under page_first the JIT dereferences the host-side k_data_ptrs table on the GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
…n screen mirrors pick_group_bytes() gained the 64/32/16 B rounds so MLA's 576 B fp8 row can reach the JIT. Those narrow rounds only pay off against the raised ROCm block quota, so guard them and leave CUDA generating exactly the code it did before. kvcache/hicache.py screens element sizes with a Python copy of that rule and derives its lane count from a literal 32 mirroring device::kWarpThreads. If the constant ever moves, the screen silently disagrees with the rule and an accepted size fails a static_assert at JIT compile time instead. Pin it where the rule lives, so the constant's next reader is sent here.
_tiles_across_lanes() in kvcache/hicache.py is a Python copy of pick_group_bytes() in kvcacheio/hicache.cuh, and the two have to answer alike: a size the screen admits reaches a static_assert, and a size it wrongly rejects silently loses the JIT path. Sweep element sizes against the rule on both platforms, and name the 576 B MLA fp8 row the narrow rounds exist for -- admitted on ROCm, still turned away by CUDA's 128-only screen.
372ecff to
339c0fa
Compare
|
Updated this: made the 32-thread copy group explicit (it is not a hardware warp), added compile-time checks, and added AMD CI coverage for the 576B path. Could you take another look? |
|
@DarkSharpness can you help do a final pass on the kernel changes? |
…ool (sgl-project#37152) Squashed from sgl-project/sglang PR sgl-project#37152 (open, not yet merged). Three changes on top of sgl-project#35233 (already in this base as 0163f8f): - pick_group_bytes() picks the widest of 128/64/32/16 B that divides the element and splits across lanes into a 4/8/16 B package, so element sizes not divisible by 128 (MLA's 576 B fp8 row) can use the JIT transfer kernels. Narrow rounds sit behind #ifdef USE_ROCM; the #else path reduces to the prior 128 B rule, so CUDA behaviour is unchanged. - can_use_hicache_jit_kernel() screens on the same rule via _tiles_across_lanes() instead of element_size % 128. - MHATokenToKOnlyPoolHost.can_use_jit now admits HIP, not CUDA only. The ROCm block quota is NOT changed by this PR. DEFAULT_BLOCK_QUOTA is already 32 on HIP and 2 on CUDA at this base (kvcache/hicache.py:21); the PR only carries it as context. An earlier version of this message credited the PR with a 2 -> 16 change, which is wrong on both counts. Original commits: 3a82fcf [ROCm] Let MLA fp8 rows reach the HiCache read JIT ab96bbb [ROCm] Enable HiCache JIT transfer kernels for MHATokenToKOnlyPoolHost dddcda8 [ROCm] Keep CUDA on the 128 B round, and pin the lane count the Python screen mirrors 2efb2ab [ROCm] Test that the copy-round screen agrees with the kernel rule 339c0fa [ROCm] Clarify HiCache logical copy groups (two "Merge branch 'main' into rocm-hicache" commits dropped; applied as the PR's net diff against its fork point 923e4a5) Co-authored-by: Xiaobo Chen <xiaobo.chen@amd.com>
# Conflicts: # python/sglang/kernels/jit/csrc/kvcacheio/hicache.cuh
Depends on #35233. That PR makes registered host pointers usable from the
transfer kernels on ROCm; this one is the part that decides how much of the KV
cache can actually take that path, and how fast it moves once it does. An
earlier revision of this PR carried its own pointer fix (pin_memory allocation,
an unregister guard, and an io-backend downgrade for storage-owned memory);
those are dropped in favour of #35233, which solves the same problem more
generally and does not have to give up the kernel backend for storage-owned
pools.
What is left here
Copy rounds narrower than 128 B. The JIT transfer kernels tiled each element
in 128 B rounds only, so an element size 128 does not divide could not use them
at all. MLA's fp8 row is 576 B.
pick_group_bytes()now takes the widest roundof 128/64/32/16 that both divides the element and splits across the lanes into a
package the hardware has (4, 8 or 16 B).
A block quota that suits ROCm. 2 on CUDA, 16 on ROCm. The per-layer read path
loses bandwidth below that, which is also why the narrow rounds are worth their
extra instantiations only on ROCm.
MHATokenToKOnlyPoolHostcan use the JIT path.can_use_jitaccepted CUDAonly; the kernels are not CUDA-specific.
CUDA is unchanged
Not "verified by inspection" — the narrow rounds are behind
#ifdef USE_ROCM,and the
#elsebranch reduces togroup_fits(bytes, num_threads, 128), whosethree conditions are the two original
static_asserts plus the package-sizeconstraint
PackageTypealready imposed.DEFAULT_BLOCK_QUOTAandGROUP_BYTESare gated the same way in Python. Non-ROCm builds generate the code they did
before.
Tests
test/registered/unit/mem_cache/test_hicache_copy_rounds.py, CPU-only, inbase-a-test-cpu.kvcache/hicache.pyscreens element sizes with a Python copyof
pick_group_bytes(), and the two have to answer alike: a size the screenwrongly admits fails a
static_assertat JIT compile time, and one it wronglyrejects silently loses the JIT path. The test sweeps element sizes against the
rule on both platforms and names 576 B as the boundary the narrow rounds exist
for. 5 cases, 963 subtests.
The lane count the screen derives from is a literal 32 mirroring
device::kWarpThreads;kvcacheio/hicache.cuhnowstatic_asserts thatconstant, so a wavefront-size change fails at the rule instead of silently
disagreeing with the screen.
Measurements
TODO: re-measure on top of #35233. The numbers in the previous revision of this
description were taken with the pointer fix that has since been dropped, so they
do not describe this change.
ruff (F401,F821,UP037), ruff-format and isort clean.
CI States
Latest PR Test (Base): ✅ Run #35396160706
Latest PR Test (Extra): ✅ Run #35396160406
Latest PR Test (AMD ROCm 10): ❌ Run #35396160667