Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a fused CUDA kernel that combines Rotary Positional Embedding (RoPE) with the KV cache update process for the flash NHD layout. The implementation includes the CUDA kernel, Python bindings, and integration into the FlashAttention backend. Feedback identifies two critical correctness issues: the kernel incorrectly uses integer types for floating-point conversions in the FP8 path, and the backend lacks a check to ensure the KV cache layout is NHD, which could lead to memory corruption.
| for (int i = threadIdx.x; i < n_elems; i += blockDim.x) { | ||
| const raw_kv_scalar_t k_raw = | ||
| *reinterpret_cast<const raw_kv_scalar_t*>(k_src + i); | ||
| const raw_kv_scalar_t v_raw = | ||
| *reinterpret_cast<const raw_kv_scalar_t*>(v_src + i); | ||
| k_dst[i] = fp8::scaled_convert<cache_t, raw_kv_scalar_t, kv_dt>( | ||
| k_raw, k_scale_val); | ||
| v_dst[i] = fp8::scaled_convert<cache_t, raw_kv_scalar_t, kv_dt>( | ||
| v_raw, v_scale_val); | ||
| } |
There was a problem hiding this comment.
The kernel passes raw_kv_scalar_t (which is an integer type like uint16_t for FP16/BF16 inputs) to fp8::scaled_convert. This causes the conversion to perform integer-to-float casting on the bit representation of the RoPE output, resulting in incorrect values. It should use the floating-point type qk_t and access elements directly via k_src[i] and v_src[i] to ensure correct floating-point conversion.
for (int i = threadIdx.x; i < n_elems; i += blockDim.x) {
k_dst[i] = fp8::scaled_convert<cache_t, qk_t, kv_dt>(
k_src[i], k_scale_val);
v_dst[i] = fp8::scaled_convert<cache_t, qk_t, kv_dt>(
v_src[i], v_scale_val);
}
| # NVFP4 KV cache (SM100+) uses a different write path; the fused kernel | ||
| # in this PR covers auto / fp8_e4m3 / fp8_e5m2 only. Other variants | ||
| # (per-token-head FP8, HND layout) are not produced by this backend. | ||
| return self.kv_cache_dtype != "nvfp4" |
There was a problem hiding this comment.
The fused_rope_kvcache_supported method assumes that the HND layout is not produced by this backend. However, FlashAttentionBackend supports both NHD and HND layouts depending on configuration. The fused kernel implementation in rope_kvcache_fusion_kernels.cu assumes a contiguous [num_kv_heads, head_size] block per slot, which is only true for the NHD layout. Running this on an HND layout will lead to silent memory corruption. An explicit check for get_kv_cache_layout() == "NHD" should be added.
| return self.kv_cache_dtype != "nvfp4" | |
| return self.kv_cache_dtype != "nvfp4" and get_kv_cache_layout() == "NHD" |
99c7fdb to
b5aff8f
Compare
b5aff8f to
6046e65
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
6046e65 to
44f84e6
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
After a sweep, this script now answers "what should I optimize by hand?"
on top of the existing compile-time/match-count summary:
- PASS_INFO knowledge base maps each fusion pass to: what it fuses,
its FX pattern file, the manual-kernel status (PR vllm-project#43355-style),
and the pass_config gate that enables it.
- Parses 'pass_config': {...} and 'splitting_ops': [...] from the
engine config log line and surfaces ON/OFF gate state.
- Diagnoses the no-match case: distinguishes "pass_config not seen"
(DEBUG off?) from "pass_config: {} = all defaults" (bf16 + TP=1
Granite 4 — no gate enabled, so no fusion can fire).
- Sorts observed passes by leverage (HIGH/MEDIUM/LOW), deprioritizes
ROCm-only passes on a CUDA dump, and explains every unseen pass
using its actual gate state.
- Footer is the manual-fusion template from PR vllm-project#43355 / issue vllm-project#43224
(kernel -> torch.ops binding -> call-site rewire -> drop FX pass).
Usage unchanged:
./parse_compile_dump.py <server_log> <dump_dir>
python parse_compile_dump.py \
bench_results/compile_sweep/granite-4.0-h-small/logs/server_full.log \
bench_results/compile_sweep/granite-4.0-h-small/compile_dump
Signed-off-by: Francesco Fusco <ffu@zurich.ibm.com>
44f84e6 to
bae76ce
Compare
bae76ce to
5e4aa81
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
|
Hi @A1c0r-Z @mgoin — I independently started working on this CUDA RoPE + KV-cache fusion gap against current vLLM main before discovering this PR and #43503. I do not want to open a competing PR without coordinating here first. My current-main prototype follows the same broad native-kernel + FlashAttention integration direction, but the port surfaced two correctness/integration details that may be useful to this effort:
I also have current-main SM89 operator benchmarks and compile/V2 PIECEWISE route evidence, and I am continuing to turn the prototype into an upstream-reviewable candidate. Which continuation would you prefer?
I am happy to adapt to maintainer direction. In the meantime I will keep validating the current-main implementation locally rather than opening a duplicate PR. |
|
Correction to the FP8 point in my comment above: after tracing this PR's exact The |
…lash_attn Part of vllm-project#43224 (the rope + kv_cache_write bullet under the CUDA fusion-pass inventory). On NVIDIA today, RopeKVCacheFusionPass is effectively a no-op: no CUDA AttentionImpl subclass implements fused_rope_kvcache_supported(), and the pass_config.fuse_rope_kvcache flag is gated is_rocm()-only. This commit closes both gaps: 1. Adds a native CUDA fused kernel (csrc/rope_kvcache_fusion_kernels.cu) that performs in-place RoPE on Q and K, then writes K/V into the flash paged KV cache. Supports bf16/fp16 inputs with auto/fp8_e4m3/fp8_e5m2 cache, both NEOX and non-NEOX, flash NHD layout, per-tensor scales. RoPE math is done in fp32 to match the precision of apply_token_rotary_embedding, so the fused op is bit-identical to the unfused rotary_embedding + reshape_and_cache_flash pipeline. 2. Opts FlashAttentionImpl into RopeKVCacheFusionPass via fused_rope_kvcache_supported / do_rope_and_kv_cache_update overrides. 3. Relaxes the platform gate in vllm/config/compilation.py from is_rocm() to is_cuda_alike() so CUDA users can opt in. The flag still defaults to off; users must explicitly set compilation_config.pass_config.fuse_rope_kvcache=True together with splitting_ops=[] (or use_inductor_graph_partition=True). Adds a 64-case parameterized correctness test (rtol=0, atol=0 across the dispatch matrix) and a benchmark. On H200 with 2000 iters / 50 warmup, every benchmark cell is >=1.03x; decode at small/medium N is consistently 1.6-1.77x, prefill on GQA is 1.4-1.7x, MHA-at-large-N is HBM-bandwidth bound (1.03-1.07x, matching the ~10% K-re-read savings ceiling). AI assistance was used during implementation. The submitting human read every changed line, ran the unit tests and benchmark locally, and is responsible for defending the change end-to-end under maintainer review. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: cezhao <alcor_zhao@outlook.com>
5e4aa81 to
04f1121
Compare
|
Closing this. Two reasons, both structural rather than fixable by a rebase. The KV-cache layout moved. More importantly it is on the wrong side of #43503. This PR is Step 1 — filling the backend hook so Superseded by #52363. |
[Kernel] Add native CUDA fused RoPE + KV cache write op, opt-in for flash_attn
Part of #43224 #43503 (the
rope + kv_cache_writebullet under the CUDA fusion-pass inventory).Why this PR
On NVIDIA today,
RopeKVCacheFusionPassis effectively a no-op: no CUDAAttentionImplsubclass implementsfused_rope_kvcache_supported(), and thepass_config.fuse_rope_kvcacheflag is gatedis_rocm()-only invllm/config/compilation.py. This PR closes both gaps:csrc/rope_kvcache_fusion_kernels.cu).FlashAttentionImplinto the existingRopeKVCacheFusionPassmachinery.is_cuda_alike()so CUDA users can opt in. The flag still defaults to off; this PR doesn't change user behavior unless someone explicitly setscompilation_config.pass_config.fuse_rope_kvcache=True.Relationship to #43224
Step 1 of two: fill the CUDA kernel gap so
RopeKVCacheFusionPassdoes something on NVIDIA. Step 2 (separate PR) will rewire model code to call the fused op directly per the #42597 pattern, then remove the FX pass. Happy to fold both into one PR if preferred.What's in scope
New kernel file, C++ bindings, Python wrapper, FlashAttentionImpl opt-in, platform-gate relax (
is_rocm()→is_cuda_alike()), parameterized correctness test, and a benchmark. See the Files changed tab.Support matrix (first PR)
scalar_t(q/k/v):bf16,fp16.cache_t:bf16,fp16,fp8_e4m3,fp8_e5m2.IS_NEOX: both branches.[num_blocks, block_size, num_kv_heads, head_size]). Strides validated by host-wrapperTORCH_CHECKs.k_scale.numel() == 1).Deferred to follow-ups
vectorize_with_alignment— tried in this PR, regressed small-N by 30–60% (register pressure / occupancy hit); reverted to scalar. Could revisit with explicituint4stores.Kernel design
min(max(rope_work, cache_work), 512). Mirrorsconcat_and_cache_mla_rope_fused_kernel's shape.__syncthreads().slot_idx >= 0, write rotated K and V into the paged cache (optionally FP8-quantizing viafp8::scaled_convert).See the header comment of
csrc/rope_kvcache_fusion_kernels.cufor design notes (fp32 RoPE math, scalar Phase-3 writes, bit-identical guarantee).How to enable
Two flags must be set together — there's already a warning in
compilation.pynear the platform gate prompting this:The two-flag opt-in is friction; unifying these into a single user-visible default is a reasonable follow-up but out of scope here.
Test plan
E2E sanity script (TinyLlama-1.1B-Chat-v1.0 +
fuse_rope_kvcache=True+splitting_ops=[], greedy decode 32 tokens on"The capital of France is") run as three subprocesses for the three-way control table below. Expected DEBUG output:Correctness assertion (bit-identical)
The test asserts
torch.testing.assert_close(..., rtol=0, atol=0)across the matrix:dtype_cache∈ {(bf16, auto), (fp16, auto), (bf16, fp8_e4m3), (fp16, fp8_e4m3)}head_config∈ {MHA(32,32,128), GQA(32,8,128)}is_neox∈ {True, False}num_tokens∈ {1, 8, 128, 2048}The test samples
slot_mappingwithout replacement — duplicate slots would cause "last write wins" non-determinism between the fused and unfused paths under non-deterministic CUDA block scheduling. The production v1 scheduler is already constrained to produce unique non-negative slot ids within a single forward pass (each request gets a disjoint block allocation; prefix-cache sharing is read-only; CUDA-graph padding uses-1which our kernel skips). Our kernel inherits the same uniqueness assumption as the existingreshape_and_cache_flash— no new exposure.Benchmark — H200, 2000 iters / 50 warmup, median-of-3 independent runs
Every cell ≥ 1.03×. Sweet spot is decode at small/medium N (consistent 1.6–1.77×) and prefill on GQA (1.4–1.7×). The MHA-at-large-N row (1.03–1.07×) is HBM-bandwidth-bound — fusion only saves the K re-read between the two unfused kernels, which is ~10% of total traffic on MHA; the measurement matches that physical ceiling.
E2E sanity (three-way control)
TinyLlama-1.1B-Chat,
CUDA_VISIBLE_DEVICES=1, greedy decode (temperature=0, seed=0), prompt"The capital of France is", 32 generated tokens. Three configs run as separate subprocesses for clean isolation.fuse_rope_kvcachesplitting_ops' Paris.\n\n2. B. The capital of France is Paris...'[]' Paris.\n\n2. B. C. The capital of Canada is Ottawa...'[]split_onlyMarker counts were obtained via temporary NVTX/print instrumentation in
do_rope_and_kv_cache_updateduring validation; that instrumentation is not part of the submitted code.split_only == fulltoken-for-token; the divergence frombaselineis entirely caused bysplitting_ops=[](inductor sees a different graph → different fusion choices → different reduction orders in attention → small fp diffs accumulate over autoregressive decoding). The fused op is bit-identical to the unfused op when the rest of the compile config is held constant. The marker count = 1540 confirms the runtime call path is live (~70 forward passes × 22 decoder layers).What I haven't done in this PR