Skip to content

model : optimize DSA RoPE, remove some ggml_concat - #27091

Closed
ngxson wants to merge 2 commits into
ggml-org:masterfrom
ngxson:xsn/dsa_optim_rope
Closed

model : optimize DSA RoPE, remove some ggml_concat#27091
ngxson wants to merge 2 commits into
ggml-org:masterfrom
ngxson:xsn/dsa_optim_rope

Conversation

@ngxson

@ngxson ngxson commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Overview

DSA indexer head layout is:

  • [rope | nope]: used by ds3.2 and glm-dsa --> now simply use n_rot instead of ggml_concat
  • [nope | rope]: used by dsv4 --> now requires building freq_factors as input, still cheaper than doing ggml_concat on large tensors

Next PR: apply same fix to dflash

Requirements

@ngxson
ngxson requested a review from CISC as a code owner August 14, 2026 22:12
@ngxson
ngxson requested review from am17an and fairydreaming August 14, 2026 22:13
@github-actions github-actions Bot added the model Model specific label Aug 14, 2026
@ngxson
ngxson marked this pull request as draft August 14, 2026 22:45
@ngxson

ngxson commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

thanks @AesSedai for testing, the results with dsv4-flash:

master - build 16d222f (10437)

model size params backend ngl n_ubatch test t/s
deepseek4 ?B MXFP4 MoE 150.75 GiB 284.33 B CUDA -1 2048 pp2048 2552.04 ± 16.28
deepseek4 ?B MXFP4 MoE 150.75 GiB 284.33 B CUDA -1 2048 tg128 55.52 ± 0.09

PR - build 0ef788f (10433)

model size params backend ngl n_ubatch test t/s
deepseek4 ?B MXFP4 MoE 150.75 GiB 284.33 B CUDA -1 2048 pp2048 2565.17 ± 14.99
deepseek4 ?B MXFP4 MoE 150.75 GiB 284.33 B CUDA -1 2048 tg128 56.36 ± 0.08

Gain

test master PR gain significance
pp2048 2552.04 ± 16.28 2565.17 ± 14.99 +0.51% 1.9 sigma
tg128 55.52 ± 0.09 56.36 ± 0.08 +1.51% 22.1 sigma

however, note that it comes with a drift because we are now having to calculate rope freq_factors in a hacky way:

Drift by position (4096-token run):

pos    0- 256:  NMSE 3.0e-07   top-1 100.00%
pos  256-1024:  NMSE 1.0e-05   top-1  99.87%
pos 1024-2048:  NMSE 3.4e-05   top-1  99.80%
pos 2048-3072:  NMSE 1.6e-05   top-1  99.80%
pos 3072-4096:  NMSE 2.2e-05   top-1  99.71%

Verdict

1.5% perf gain could worth it, but probably better to ask users to reconvert gguf, rather than adding more hacks to dsv4

nevertheless, the dsv3.2 and glm-dsa case are applied cleanly

Comment thread src/models/deepseek4.cpp
ggml_row_size(comp->type, n_embd_head_nope));

comp_pe = ggml_rope_ext(ctx0, comp_pe, comp_pos, nullptr, n_embd_head_rope, rope_type, n_ctx_orig,
comp = build_rope_trail(comp, comp_pos, n_embd_head, n_embd_head_rope, n_ctx_orig,

@fairydreaming fairydreaming Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you instead keep the comp_pe view and do ggml_rope_ext_inplace() on it? This avoids the concat too, but I guess you'd need to call ggml_build_forward_expand() on the result.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ggml_rope_ext_inplace is tricky because it will break if tensor and the ops are not on the same device, I ran into the exact same problem back then with 2D RoPE

I think it's better to just add a new n_offset param to rope kernel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ggml_rope_ext_inplace is tricky because it will break if tensor and the ops are not on the same device, I ran into the exact same problem back then with 2D RoPE

I think it's better to just add a new n_offset param to rope kernel

@ngxson Perhaps a boolen flag would be enough (what to RoPE - head or tail).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ggml_rope_ext_inplace is tricky because it will break if tensor and the ops are not on the same device, I ran into the exact same problem back then with 2D RoPE
I think it's better to just add a new n_offset param to rope kernel

@ngxson Perhaps a boolen flag would be enough (what to RoPE - head or tail).

@ngxson A better idea - if n_dims is negative then rope tail, otherwise rope head. No need to change the API (lazy way is the best way).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that will still require changing the kernel anyway, the API can be just ggml_rope_set_offset(op, offset), similar to ggml_mul_mat_set_prec, so no breaking change in API

also having offset will allow much flexible usage, like for example if there are 3 rope sections (for example, 3D rope), the middle section can be controlled using the combination of offset+n_rot

@fairydreaming

fairydreaming commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Anyway I think that DeepSeek V3.2 and GLM changes are fine (I have no idea who wrote that crappy code 😉), I think we can merge them right now, so perhaps it would be best to split this into two PRs? (looks like the offset part will still take some time)

@CISC

CISC commented Aug 15, 2026

Copy link
Copy Markdown
Member

I think we can merge them right now, so perhaps it would be bet to split this into two PRs? (looks like the offset part will still take some time)

👍

@ngxson

ngxson commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

closing this and will replace with another PR after #27120 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

model Model specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants