llama: fix quantized kv-cache for dsv4#25202
Conversation
| static constexpr uint32_t DSV4_HCA_RATIO = 128; | ||
| // [TAG_DSV4_CACHE_PAD] | ||
| // matches MATRIX_ROW_PADDING used by backends for quantized row padding | ||
| static constexpr uint32_t DSV4_CACHE_PAD = 512; |
There was a problem hiding this comment.
What's the reason for using this padding? I get it that it matches MATRIX_ROW_PADDING, but it seems to be only used internally by backends, so I don't understand why is it needed.
There was a problem hiding this comment.
If I didn't add this padding it would crash because quantized tensors need that padding. Not sure if there's a better way to do this
There was a problem hiding this comment.
How to replicate this crash? Tried this PR without padding changes (llama-kv-cache-dsv4.cpp file unchanged) with CUDA backend and can't get it to fail.
Edit: maybe it's related to me using CPU expert offloading. I see more graph splits with quantized KV cache.
There was a problem hiding this comment.
Are you using a quantized cache?
There was a problem hiding this comment.
Are you using a quantized cache?
Sure, tried with --cache-type-k q8_0 --cache-type-v q8_0.
There was a problem hiding this comment.
Besides, from what I see in backends code MATRIX_ROW_PADDING applies to ne0 dimension, how KV cache length would appear there?
There was a problem hiding this comment.
I checked why I have more graph splits with quantized KV cache and it's caused by GGML_OP_CONCAT of Q8_0 KV cache (when csa_k_all tensor is created), so I don't think it's related to the crash you mentioned. But these concats will run on CPU when using quantized cache.
|
I can confirm that this resolves the gibberish text. Comparing to b9840 I was getting nonsense output and on this commit it works fine. Command line: |
|
@am17an let me know if you have any reproducible case that shows the crashes you mentioned related to I posted info about my dsv4 branch (that among other fixes includes this PR and PRs for quantized CONCAT) on Reddit few days ago. I omitted all changes in I don't know, maybe some specific combination of options is needed to trigger it. I'd like to investigate it more before this is merged. Edit: mystery solved, see #25851 |
|
If the quantized kv-cache works after your changes I'm ok closing this one |
@am17an No, I used this PR and it works, but without the changes in |
|
Ah okay, let me revert those changes then. If I face the crash again I'll take a better look |
fairydreaming
left a comment
There was a problem hiding this comment.
For me it's good to merge in this state, we can fix any remaining problems with padding if/when they appear.
Overview
DSV4's compressed caches (CSA and HCA) did not respect the hadamard rotation when quantized kv-cache is used. This PR adds that, a side effect being that concat'ed caches need to be %512, so some extra padding is required.
Additional information
Requirements