Repository navigation
Feature: Added LIGHTNING_INDEXER support for Deepseek V4 ops on Vulkan Backend - #27453
Conversation
…ane dot-product reduction moved from a shared-memory tree to subgroupAdd.
|
Hi @shenron0101, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
|
Tried this out on a Strix Halo (AMD gfx1151). Used model 180/180 passed, no problem on No apparent change in performance or perplexity as compared to current master. |
|
Possible explanation for the null result @cdanis measured on gfx1151: most of what indexer fusion can remove on this hardware already went away when the 0<->2 permuted Same box (gfx1151 / RADV), DeepSeek-V4-Flash IQ3_XXS, 9040-token prompt (indexer
In absolute terms the glue went 67.5 s -> 3.6 s of an 87.7 s prefill. That caps indexer fusion at roughly +4.3% prefill / +0.5% decode at this context length, and the fused kernel also has to absorb the indexer matmul (1.01% of prefill), which is a tuned Two caveats that cut the other way: this is n=1 at a single context length, and the indexer glue is O(n_kv) while the HC ops are O(1), so the indexer's share grows with depth. If this change is going to show up anywhere it is at depth — d24576 or a 32k prompt — rather than at the default. Worth measuring there before treating the null as final. Minor: the HC ops referenced in the description are #26578 (reopened today), not #26548, which was superseded. |
|
I wasn't too surprised with the null result given the lack of fused HC ops. I'll try again combined with the re-opened PR, thanks :) |
|
|
||
| layout (binding = 1) readonly buffer K_PACKED_BF16 { u16vec4 data[]; } k_packed_bf16; | ||
|
|
||
| #ifndef FA_K_ONLY |
There was a problem hiding this comment.
I don't think this is needed, unused bindings make no difference and you'd just need something like #define FaTypeV 0 to avoid the compile issue.
jeffbolznv
left a comment
There was a problem hiding this comment.
The ggml-vulkan changes look mostly good to me, I just had a couple minor comments. I didn't do a detailed review of the test changes. I'm surprised such large changes were needed, but I didn't look closely.
| barrier(); | ||
|
|
||
| float score = 0.0; | ||
| for (uint h = 0; h < n_heads; ++h) { |
There was a problem hiding this comment.
Looks like each head is independent, so there's probably a lot of opportunity to optimize. But this version is clear and fine for the initial implementation.
There was a problem hiding this comment.
Tested a version with optimizations that we can add if needed.
| Shape | Baseline | Optimized | Speedup |
|---|---|---|---|
| nh=32 kv=256 | 464.0 GFLOPS | 486.2 GFLOPS | 1.05x |
| nh=64 kv=256 | 469.0 GFLOPS | 497.3 GFLOPS | 1.06x |
| nh=32 kv=4096 | 468.9 GFLOPS | 559.4 GFLOPS | 1.19x |
| nh=64 kv=4096 | 473.8 GFLOPS | 576.2 GFLOPS | 1.22x |
| nh=32 kv=65536 | 461.3 GFLOPS | 563.1 GFLOPS | 1.22x |
| nh=64 kv=65536 | 467.3 GFLOPS | 568.9 GFLOPS | 1.22x |
There was a problem hiding this comment.
Shall I leave this as a seperate PR?
There was a problem hiding this comment.
Yes, leave for a future change
This reverts commit fdcbdd9.
|
@ggml-org/ggml-cuda CUDA is failing these new tests. Not sure about the other CI issues. |
jeffbolznv
left a comment
There was a problem hiding this comment.
I haven't reviewed the details of the test changes, but the ggml-vulkan code LGTM.
|
The test changes look like the friendly AI agent noticed that in real life K tensor is non-contiguous and decided to test non-contiguous inputs in general, so it made the necessary changes for this and some additional friendly refactors all over the place. While the intention is admirable, the execution is a bit confusing, so personally I'd purge it with fire, maybe except the part where |
|
IMO it would be better to fix the supports_op for the other backends. |
|
test-backend-ops changes look weird. It's just a normal op, no need to add these tests. We've been running this with the model which uses it for quite some time. |
@jeffbolznv OK, I'm going to investigate the problem for CUDA now, let's talk about what to change in tests later. |
|
From what I see in logs, the failing cases all follow the pattern where there is NaN present both in CPU and in tested backend, examples: CUDA: Also they are NONE tensors (sentinels?), so likely the problem is that this new test case doesn't initialize sentinels and sometimes their elements are randomly NaN. I see that My opinion is unchanged - these new cases shall be removed, we should focus on testing realistic inputs used by models (non-contiguous KQ mask doesn't seem be to such case, AFAIK q and weights are contiguous too). If we want to add tests with k as a view we should follow the |
|
Makes sense. @shenron0101 please refactor the tests. What was the reason for the changes? |
|
Yup, it's as @fairydreaming mentioned. The Agent picked up that K may be a view/non-contiguous in a real model. I have reverted the test cases and I'll leave the |
…n Backend (ggml-org#27453) * vulkan: add LIGHTNING_INDEXER op * vulkan: updated lightning_indexer.comp and ggml-vulkan.cpp with 128-lane dot-product reduction moved from a shared-memory tree to subgroupAdd. * vulkan: cleanup; Skip bounds checks * vulkan: cleanup FA_K_ONLY * Revert "vulkan: cleanup FA_K_ONLY" This reverts commit fdcbdd9. * vulkan: restore interleaved K/V buffer ordering * vulkan: Remove FA_K_ONLY * vulkan: Revert flash_attn_dequant * vulkan: Revert tests in backend-ops.cpp
…n Backend (ggml-org#27453) * vulkan: add LIGHTNING_INDEXER op * vulkan: updated lightning_indexer.comp and ggml-vulkan.cpp with 128-lane dot-product reduction moved from a shared-memory tree to subgroupAdd. * vulkan: cleanup; Skip bounds checks * vulkan: cleanup FA_K_ONLY * Revert "vulkan: cleanup FA_K_ONLY" This reverts commit fdcbdd9. * vulkan: restore interleaved K/V buffer ordering * vulkan: Remove FA_K_ONLY * vulkan: Revert flash_attn_dequant * vulkan: Revert tests in backend-ops.cpp
Overview
Updated PR from the previous closed one.
LIGHTNING_INDEXER: Added support for F32, F16, BF16, and several quantized formats.
The Lightning Indexer has only been implemented in the CPU backend, CUDA backend but not in the Vulkan backend. This is an implementation of the Indexer for DeepSeek V4 on the Vulkan backend.
Assisted-by: OpenCode
Currently the Hyper-connection Operations for the Vulkan backend are being implemented by @kh0pper. The one missing operation in the PR was the lightning indexer.
##26585 — "vulkan: tiled transpose for 0<->2 permuted CONT"
The original PR which was closed:
##26548 — "vulkan: add DeepSeek-V4 hyper-connection fused ops (DSV4_HC_COMB/PRE/POST)".
Additional information
Numbers (RTX 3090, test-backend-ops perf, K=f16)
Correctness evidence
Requirements