Skip to content

model: use ggml_rope_set_offset() - #27382

Merged
ngxson merged 3 commits into
masterfrom
xsn/ggml_rope_off_use
Aug 21, 2026
Merged

ngxson merged 3 commits into
masterfrom
xsn/ggml_rope_off_use

Conversation

@ngxson

@ngxson ngxson commented Aug 19, 2026 •

Copy link
Copy Markdown
Collaborator

Overview

Supersede #27091

May depend on #27345

Tested with deepseekv4 gguf and confirmed to output bit-exact as master

Requirements

@github-actions github-actions Bot added documentation Improvements or additions to documentation model Model specific labels Aug 19, 2026
@ngxson
ngxson marked this pull request as ready for review August 20, 2026 22:38
@ngxson
ngxson requested a review from CISC as a code owner August 20, 2026 22:38
@ngxson
ngxson requested a review from fairydreaming August 20, 2026 22:38

@CISC CISC left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't this be applied to deepseek2, deepseek32, glm-dsa and kimi-k3/linear MLA as well?

@ngxson

ngxson commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Upon closer look, unfortunately some models still genuinely need concat, this is because the logic looks like this:

q_nope = ggml_permute(ctx0, q_nope, 0, 2, 1, 3);
ggml_tensor * q_nope_absorbed = ggml_mul_mat(ctx0, layer.wk_b, q_nope);
q_nope_absorbed = ggml_permute(ctx0, q_nope_absorbed, 0, 2, 1, 3);

ggml_tensor * Q = ggml_concat(ctx0, q_nope_absorbed, q_pe, 0);

While it's still [nope|rope], the nope part need to go through an extra projection. So the problem now become "can we do mul_mat inplace" which I think not worth optimizing that far.

An interesting finding though, kimi-k3 doesn't use rope at all, despite using the term "rope" in variable name, likely because that part of code is copied from deepseek MLA.

Added this to docs:

Exception: some models apply an extra op to the nope part, for example deepseek32.cpp, and may not use this optimization. While RoPE can be applied selectively to a part of the head, the extra op may not, so these models still need views plus ggml_concat.

@ngxson
ngxson merged commit 873e5d8 into master Aug 21, 2026
23 of 26 checks passed
therealkenc pushed a commit to therealkenc/llama.cpp that referenced this pull request Aug 24, 2026
* model: use ggml_rope_set_offset()

* partially apply to deepseek2
@fairydreaming

Copy link
Copy Markdown
Contributor

Looks like this change increased compute buffer sizes, added some logs with galloc debug info in #27680

@fairydreaming

Copy link
Copy Markdown
Contributor

@ngxson On a different note - shouldn't ggml_rope_set_offset() be present in context shift graph for models that need it? Or perhaps they all don't support context shift?

@gopinath87607

Copy link
Copy Markdown

@ngxson On a different note - shouldn't ggml_rope_set_offset() be present in context shift graph for models that need it? Or perhaps they all don't support context shift?

hi will it solve the unnnerrry buffer size issues if i remove this pr from the main because i have 4 gbu and i cant use it properly due to this uneven buffer size issues

thecodacus pushed a commit to thecodacus/llama.cpp that referenced this pull request Sep 7, 2026
* model: use ggml_rope_set_offset()

* partially apply to deepseek2
zbrad pushed a commit to zbrad/llama.cpp that referenced this pull request Sep 10, 2026
* model: use ggml_rope_set_offset()

* partially apply to deepseek2
pl752 pushed a commit to pl752/llama.cpp that referenced this pull request Sep 15, 2026
* model: use ggml_rope_set_offset()

* partially apply to deepseek2
frostyautumnleaf pushed a commit to frostyautumnleaf/llama.cpp that referenced this pull request Oct 5, 2026
* model: use ggml_rope_set_offset()

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

Labels

documentation Improvements or additions to documentation model Model specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants