Skip to content

metal : add sparse FA - #28098

Merged
ggerganov merged 10 commits into
masterfrom
gg/metal-fa-sparse
Sep 3, 2026
Merged

metal : add sparse FA#28098
ggerganov merged 10 commits into
masterfrom
gg/metal-fa-sparse

Conversation

@ggerganov

@ggerganov ggerganov commented Aug 31, 2026

Copy link
Copy Markdown
Member

Overview

cont #27970

Add Metal support for sparse Flash Attention.

Results for DSv4 on M2 Ultra:

  • Before
|    PP |     TG |    B |   N_KV |   T_PP s | S_PP t/s |   T_TG s | S_TG t/s |      T s |    S t/s |
|-------|--------|------|--------|----------|----------|----------|----------|----------|----------|
|  2048 |     32 |    1 |   2080 |    5.100 |   401.57 |    1.164 |    27.50 |    6.264 |   332.06 |
|  4096 |     32 |    1 |   4128 |   10.900 |   375.77 |    1.189 |    26.92 |   12.089 |   341.47 |
|  8192 |     32 |    1 |   8224 |   25.316 |   323.58 |    1.233 |    25.95 |   26.550 |   309.76 |
| 16384 |     32 |    1 |  16416 |   65.888 |   248.67 |    1.307 |    24.48 |   67.195 |   244.30 |
| 32768 |     32 |    1 |  32800 |  192.359 |   170.35 |    1.407 |    22.75 |  193.765 |   169.28 |
| 65536 |     32 |    1 |  65568 |  612.039 |   107.08 |    1.572 |    20.36 |  613.611 |   106.86 |
  • After
|    PP |     TG |    B |   N_KV |   T_PP s | S_PP t/s |   T_TG s | S_TG t/s |      T s |    S t/s |
|-------|--------|------|--------|----------|----------|----------|----------|----------|----------|
|  2048 |     32 |    1 |   2080 |    5.107 |   401.01 |    1.178 |    27.15 |    6.286 |   330.92 |
|  4096 |     32 |    1 |   4128 |   10.668 |   383.94 |    1.190 |    26.88 |   11.859 |   348.10 |
|  8192 |     32 |    1 |   8224 |   21.934 |   373.49 |    1.202 |    26.62 |   23.135 |   355.47 |
| 16384 |     32 |    1 |  16416 |   45.233 |   362.21 |    1.219 |    26.26 |   46.452 |   353.40 |
| 32768 |     32 |    1 |  32800 |   94.200 |   347.85 |    1.256 |    25.48 |   95.456 |   343.61 |
| 65536 |     32 |    1 |  65568 |  202.556 |   323.54 |    1.342 |    23.84 |  203.899 |   321.57 |

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES, pi:llama.cpp/DeepSeek-v4-0731 + pi:llama.cpp/Qwen3.8-27B

@github-actions github-actions Bot added model Model specific testing Everything test related ggml changes relating to the ggml tensor library for machine learning Apple Metal https://en.wikipedia.org/wiki/Metal_(API) CUDA Related to the CUDA backend labels Aug 31, 2026
@ggerganov
ggerganov force-pushed the gg/metal-fa-sparse branch 2 times, most recently from cd3a488 to 6d68373 Compare August 31, 2026 18:32
@ggerganov ggerganov changed the title metal : add sparse attention (wip) metal : add sparse FA Sep 2, 2026
@ggerganov
ggerganov marked this pull request as ready for review September 2, 2026 17:12
@ggerganov
ggerganov requested review from a team and CISC as code owners September 2, 2026 17:12
@ggerganov

Copy link
Copy Markdown
Member Author

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown
Automated code review

Review complete. Here is my review of the Metal sparse Flash Attention changes.

Summary: the approach (gather finite mask entries into per-row index lists, then run the existing vec kernels over gathered rows) is sound and matches the shape of the CUDA sparse FA from #27970. The kernel gather logic in kernel_flash_attn_ext_vec correctly mirrors the dense access patterns (row from i11*nb11/nb21, columns ii*NL + tx, ss[qq*C + cc*NE + ty] pairing between the K and V phases checks out). I found one real bug and a few smaller items.

Blocking

(point 1) ggml/src/ggml-metal/ggml-metal-ops.cpp:3669 - the sparse path re-enables nwg = 1 (for ne01 > 32, i.e. the main prefill case the PR benchmarks), which makes the previously-dead assert(ggml_metal_op_flash_attn_ext_extra_tmp(op) == 0); reachable - and it is always false: ggml_metal_op_flash_attn_ext_extra_tmp() unconditionally reserves the temp buffer (if (true) at line ~3055). Any debug build aborts on the first sparse prefill with batch > 32. Remove the assert (the buffer is reserved but unused in this path), or make the extra-size functions reflect actual need. This also means the ne01 > 32 sparse path is completely untested (see point 2).

Will slow the review

(point 2) All sparse test_flash_attn_ext cases use small batches (nb 1-3, nr23 up to {16,2}), so neither the nwg == 1 path from point 1 nor the ne01 <= 32 + nwg = 32 reduce path with a sparse list is exercised at ne01 > 32. Please add cases to the existing FA matrix (no new test file) with a sparse mask and a larger batch (e.g. 64), plus one quantized-KV sparse case and ideally one with max_bias != 0 since the Metal gate, unlike the CUDA one (fattn.cu:126), does not exclude bias/softcap from the sparse path.

(point 3) The pidx offsets are computed in int32 in both kernels: fa.metal kernel_flash_attn_ext_vec_idx (((i3*ne32 + i2)*ne31 + i1)*n_kv_max_padded) and the vec kernel ((((iq3%ne33)*ne32 + ...)*ne31*n_kv_max_padded + ...), note this one is a byte offset on device const char *, so it is effectively int * sizeof(int)). The gate bounds n_kv_max <= 4096 but not ne31*ne32*ne33; with a large -ub batch and many KV heads the product can exceed INT_MAX, giving a negative offset and OOB reads of the idx buffer (garbage i11 -> OOB K/V reads). The host already sizes the buffer in size_t, so just compute the offsets in 64-bit, or bound the mask-row count in ggml_metal_op_flash_attn_ext_n_kv_max_sparse.

(point 4) Hardening: the gathered i11 values come from positions [0, ne30) of the mask and are used to index K/V rows [0, ne11) with no bounds check. This is safe only because llama.cpp always builds the mask with ne30 == ne11. A one-line op->src[3]->ne[0] == op->src[1]->ne[1] check in the sparse gate would make the invariant explicit instead of implicit.

Nits

(point 5) FC_FLASH_ATTN_EXT_VEC_IDX (ggml-metal-impl.h:110) is defined but never used - kernel_flash_attn_ext_vec_idx takes no function constants. Drop it or use it.

(point 6) src/models/qwen4exp.cpp:747-749 - commented-out code with a TODO is dead weight; per the project's conventions, keep the intent in the referenced issue (#27970) rather than committing disabled code.

(point 7) tests/test-backend-ops.cpp:194 - the relaxation n_kv_max > 1 -> > 0 has no motivating test case (nothing uses n_kv_max == 1). Either add a case that exercises it or keep the old bound, so the change is not a mystery for the next reader.

(point 8) ggml-metal-device.cpp:1581 - snprintf(name, 256, "kernel_flash_attn_ext_vec_idx") formats a constant string; a plain copy (or reusing the base/name pattern) reads better. Also, the op parameter of ggml_metal_library_get_pipeline_flash_attn_ext_vec_idx exists only for asserts - fine, but consider whether the assert on src[3] is worth the coupling.

Good to see the perf table in the description and the has_sparse flag folded into both the pipeline cache key and the function constants (avoids name collisions between dense/sparse specializations). Once point 1 is fixed and the ne01 > 32 path is covered by tests, this looks mergeable from my side.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

@tarruda

tarruda commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

M1 Ultra results:

./build/bin/llama-batched-bench -m ~/ds4flash/DeepSeek-V4-Flash-0731-IQ3_XXS-00001-of-00004.gguf -c 65568 -b 2048 -ub 2048 -ngl all -fa on -npp 2048,4096,8192,16384,32768,65536 -ntg 32 -npl 

|    PP |     TG |    B |   N_KV |   T_PP s | S_PP t/s |   T_TG s | S_TG t/s |      T s |    S t/s |
|-------|--------|------|--------|----------|----------|----------|----------|----------|----------|
|  2048 |     32 |    1 |   2080 |   10.415 |   196.65 |    1.795 |    17.83 |   12.210 |   170.36 |
|  4096 |     32 |    1 |   4128 |   21.725 |   188.54 |    1.687 |    18.97 |   23.412 |   176.32 |
|  8192 |     32 |    1 |   8224 |   44.711 |   183.22 |    1.707 |    18.75 |   46.417 |   177.18 |
| 16384 |     32 |    1 |  16416 |   92.323 |   177.46 |    1.730 |    18.50 |   94.053 |   174.54 |
| 32768 |     32 |    1 |  32800 |  192.887 |   169.88 |    1.786 |    17.92 |  194.673 |   168.49 |
| 65536 |     32 |    1 |  65568 |  434.197 |   150.94 |    1.906 |    16.78 |  436.104 |   150.35 |

- add kernel_flash_attn_ext_vec_idx: compacts finite mask entries into
  a per-row index list (Hillis-Steele scan, one threadgroup per row)
- extend vec FA kernel with optional sparse index gathering (FC slot 5)
- add host-side gate: sparse path when n_kv_max > 0, mask present,
  supported head sizes / KV types, n_kv_max <= 4096
- new buffer region extra_idx for the index list
- pipeline getter extended with has_sparse param
- add test cases: head sizes, quant types, nb>1, nr23 variants,
  sinks, ALiBi, softcap, permute, v_view_of_k, no-mask fallback

Note: multi-row (nb*nr23[1] > 1) cases still failing - rid mapping
in the store phase needs revisiting for the sparse path.

Assisted-by: pi:llama.cpp/Qwen3.8-27B
- kernel_flash_attn_ext_vec_idx: mask param is half* but nb31 is a byte
  stride, so the per-row mask offset was scaled by 2x; cast to char*
  before applying the byte strides
- kernel_flash_attn_ext_vec: sparse pidx param is char* so the per-row
  element offset was under-scaled by sizeof(int); scale it by sizeof(int)
  to get the correct byte offset
- fixes the multi-row (nb*nr23[1] > 1) sparse flash attention failures

Assisted-by: pi:llama.cpp/DeepSeek-v4-0731
The idx kernel previously read the mask row twice: once to count the finite
entries (for the prefix scan) and again to recover their positions. Since the
kernel is memory-bound, this doubled the mask traffic.

Keep the finite positions in a per-thread register array during the count
pass and write them out directly, avoiding the second mask read. A dense
mask with more than NLOCAL finite entries in a slice falls back to re-reading
the mask to write the remaining positions.

Assisted-by: pi:llama.cpp/DeepSeek-v4-0731
Measure the sparse vec FA kernel across KV sizes, n_kv_max hints and batch
sizes. Run with:

    ./build/bin/test-backend-ops -b MTL0 -o FLASH_ATTN_EXT -p "n_kv_max=[1-9]" perf

Assisted-by: pi:llama.cpp/DeepSeek-v4-0731
@ggerganov
ggerganov merged commit 7bb0fc1 into master Sep 3, 2026
31 of 35 checks passed
@ggerganov
ggerganov deleted the gg/metal-fa-sparse branch September 3, 2026 10:51
@sammcj

sammcj commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

FYI @ggerganov, for the qwen4exp TODO you added: enabling sparse FA there (passing top_k->ne[0] instead of 0 in build_attn_qsa) on Qwen3.8-Flash-Next, M5 Max 128GB, IQ4_XS, q8_0 KV, -ub 2048:

  • cold prefill, 3 x ~33k-token prompts via llama-server: 664 -> 760 t/s (+14%)
  • llama-bench -p 0 -n 32: tg @ d16384 35.0 -> 39.4, @ d32768 34.5 -> 35.1
  • temp-0 output over a 36k prompt was byte-identical with and without

Build is master (de8656b) + #27836 (MTP). Small samples size of course, but happy to test anything further or raise a PR if it helps.

AI usage disclosure: YES (just for testing this), claude:opus-5

*Edit: PR: #28349

fewtarius pushed a commit to fewtarius/CachyLLama that referenced this pull request Sep 5, 2026
* metal : support n_kv_max sparse mask hint in flash attention vec kernel

- add kernel_flash_attn_ext_vec_idx: compacts finite mask entries into
  a per-row index list (Hillis-Steele scan, one threadgroup per row)
- extend vec FA kernel with optional sparse index gathering (FC slot 5)
- add host-side gate: sparse path when n_kv_max > 0, mask present,
  supported head sizes / KV types, n_kv_max <= 4096
- new buffer region extra_idx for the index list
- pipeline getter extended with has_sparse param
- add test cases: head sizes, quant types, nb>1, nr23 variants,
  sinks, ALiBi, softcap, permute, v_view_of_k, no-mask fallback

Note: multi-row (nb*nr23[1] > 1) cases still failing - rid mapping
in the store phase needs revisiting for the sparse path.

Assisted-by: pi:llama.cpp/Qwen3.8-27B

* metal : fix sparse flash attention row addressing

- kernel_flash_attn_ext_vec_idx: mask param is half* but nb31 is a byte
  stride, so the per-row mask offset was scaled by 2x; cast to char*
  before applying the byte strides
- kernel_flash_attn_ext_vec: sparse pidx param is char* so the per-row
  element offset was under-scaled by sizeof(int); scale it by sizeof(int)
  to get the correct byte offset
- fixes the multi-row (nb*nr23[1] > 1) sparse flash attention failures

Assisted-by: pi:llama.cpp/DeepSeek-v4-0731

* cont : use sparse vec FA for prefill

* metal : single-pass flash attention sparse index compaction

The idx kernel previously read the mask row twice: once to count the finite
entries (for the prefix scan) and again to recover their positions. Since the
kernel is memory-bound, this doubled the mask traffic.

Keep the finite positions in a per-thread register array during the count
pass and write them out directly, avoiding the second mask read. A dense
mask with more than NLOCAL finite entries in a slice falls back to re-reading
the mask to write the remaining positions.

Assisted-by: pi:llama.cpp/DeepSeek-v4-0731

* tests : add perf cases for sparse flash attention prefill

Measure the sparse vec FA kernel across KV sizes, n_kv_max hints and batch
sizes. Run with:

    ./build/bin/test-backend-ops -b MTL0 -o FLASH_ATTN_EXT -p "n_kv_max=[1-9]" perf

Assisted-by: pi:llama.cpp/DeepSeek-v4-0731

* qwen4 : enable sparse attention

* cont : adjust nsg

* cont : sync test-backend-ops

* cont : disable Qwen4 for now

* cont : clean-up + tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Apple Metal https://en.wikipedia.org/wiki/Metal_(API) CUDA Related to the CUDA backend ggml changes relating to the ggml tensor library for machine learning model Model specific testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants