Skip to content

model : avoid ggml_cont_3d for fused QKV weights #15662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Sep 8, 2025
Merged

model : avoid ggml_cont_3d for fused QKV weights #15662

merged 7 commits into from
Sep 8, 2025

Conversation

ggerganov
Copy link
Member

@ggerganov ggerganov commented Aug 29, 2025 •
edited
Loading

ref #15602 (comment)

  • Avoid Vcur = ggml_cont_3d(..) when the QKV weights are merged in a single tensor
  • Make llama_kv_cache:: cpy_k and cpy_v more readable

@ggerganov ggerganov mentioned this pull request Aug 29, 2025
Open
4 tasks
@CISC CISC mentioned this pull request Aug 29, 2025
Open
4 tasks
@ggerganov ggerganov marked this pull request as ready for review September 7, 2025 17:24
@ggerganov ggerganov force-pushed the gg/model-avoid-cont3d branch from f15d515 to 3dec397 Compare September 8, 2025 06:47
CISC
CISC approved these changes Sep 8, 2025
View reviewed changes
Copy link
Collaborator

@CISC CISC left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested with CodeQwen1.5, Phi2, jina-embeddings-v3 and PLaMo2.

@CISC
Copy link
Collaborator

CISC commented Sep 8, 2025

Hmmm, https://github.com/ggml-org/ci/blob/results/llama.cpp/60/d6e7c6fd8bacac0892b8722f5d5c585139cb43/ggml-4-x86-cuda-v100/stdall#L1957

@ggerganov
Copy link
Member Author

Hmmm, https://github.com/ggml-org/ci/blob/results/llama.cpp/60/d6e7c6fd8bacac0892b8722f5d5c585139cb43/ggml-4-x86-cuda-v100/stdall#L1957

This is due to #15687

@CISC
Copy link
Collaborator

CISC commented Sep 8, 2025

Hmmm, https://github.com/ggml-org/ci/blob/results/llama.cpp/60/d6e7c6fd8bacac0892b8722f5d5c585139cb43/ggml-4-x86-cuda-v100/stdall#L1957

This is due to #15687

Ah, I get a segfault locally though at the first REPEAT test after ARGMAX.

@ggerganov
Copy link
Member Author

On my end, all tests except GET_ROWS and the new IM2COL_3D are passing.

@CISC
Copy link
Collaborator

CISC commented Sep 8, 2025 •
edited
Loading

On my end, all tests except GET_ROWS and the new IM2COL_3D are passing.

Nvm, must have been some other issue pre-rebase, I pulled latest changes and applied #15868 and everything is fine now.

Edit: Eh, almost, got GGML_ASSERT(ggml_is_contiguous(src0)) on PAD, but that's surely not related. It's pad_ext test with v == true. Fixed in #15869

@ggerganov ggerganov merged commit cf0e3ba into master Sep 8, 2025
52 of 55 checks passed
@ggerganov ggerganov deleted the gg/model-avoid-cont3d branch September 8, 2025 07:25
@CISC CISC mentioned this pull request Sep 9, 2025
Closed
njsyw1997 pushed a commit to aizip/llama.cpp that referenced this pull request Sep 10, 2025
@ggerganov @njsyw1997
model : avoid ggml_cont_3d for fused QKV weights (ggml-org#15662)
96bbb20 
* model : avoid ggml_cont_3d for fused QKV weights

ggml-ci

* kv-cache : make cpy_k and cpy_v implementation more readable

ggml-ci

* cont : add comments

ggml-ci

* cont : minor fix [no ci]

* cont : one more fix

* cont : clarity

ggml-ci

* kv-cache : require contiguous heads of k_cur and v_cur

ggml-ci
@ggerganov ggerganov mentioned this pull request Oct 3, 2025
Closed
@CISC CISC mentioned this pull request Oct 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CISC CISC CISC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ggerganov @CISC