llama : skip K/V rotation input when its buffer is unallocated - #25215
Conversation
llm_graph_input_attn_kv::set_input and llm_graph_input_attn_kv_iswa::set_input call set_input_k_rot / set_input_v_rot whenever the rotation tensor pointer is non-null, but the tensor's buffer can be unallocated (NULL) when a graph only stores K/V without attending -- e.g. DFlash speculative decoding's KV-injection pass. set_input_k_rot then calls ggml_backend_buffer_is_host() on a NULL buffer and aborts with GGML_ASSERT(buffer). Guard the four k_rot/v_rot inputs with the same "&& ->buffer" check that the adjacent kq_mask inputs already use in these two functions. When the buffer is unallocated there is no data to upload, so skipping is correct. Fixes ggml-org#25191 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
ruixiang63
left a comment
There was a problem hiding this comment.
As long as we confirm this resolve the linked issue, I think it should be good to merge.
| } | ||
|
|
||
| if (self_v_rot_swa) { | ||
| if (self_v_rot_swa && self_v_rot_swa->buffer) { |
There was a problem hiding this comment.
All these changes are the same as set_input_kq_mask, so it looks good to me.
|
Thanks @bestbug456 for building and verifying the fix in #25191. @ruixiang63 since this is in the DFlash path you authored, I'll defer to you — happy to have #25215 merged, or to close it if you'd prefer folding the guard into the DFlash work directly. Whatever suits you best. |
|
@ruixiang63 without this PR, is the quantized kv-cache with dflash broken atm? |
Yes, as described in this issue: #25191 The reason could be the KV cache injection in DFlash which doesn't need to attend attention only does injection. I added the similar guard in Line 493 in ef2d770 |
|
BTW, with a quantized KV cache for the DFlash draft model, the acceptance rate drops by less than 1% in my test case. I think this is a valid fix, but I wouldn't recommend it as the preferred way to run DFlash spec dec. |
…gml-org#25215) llm_graph_input_attn_kv::set_input and llm_graph_input_attn_kv_iswa::set_input call set_input_k_rot / set_input_v_rot whenever the rotation tensor pointer is non-null, but the tensor's buffer can be unallocated (NULL) when a graph only stores K/V without attending -- e.g. DFlash speculative decoding's KV-injection pass. set_input_k_rot then calls ggml_backend_buffer_is_host() on a NULL buffer and aborts with GGML_ASSERT(buffer). Guard the four k_rot/v_rot inputs with the same "&& ->buffer" check that the adjacent kq_mask inputs already use in these two functions. When the buffer is unallocated there is no data to upload, so skipping is correct. Fixes ggml-org#25191 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> (cherry picked from commit a410713)
|
Hey @ruixiang63, quick question — I have a fix ready #25670 but can't mark it "Ready for review"; getting a permission error on markPullRequestReadyForReview. Any idea what's going on, or who I should ask? Thanks. |
|
no idea what's going on. It looks only you can mark the PR ready. Maybe an alternative way is to create a new PR and close the old one. |
|
Okay it seems you have reached the PR limit, do you have any other open PRs? Please prioritize your open PRs and only mark one ready. |
Okay, Thanks. |
|
It says "author has reached PR limit", so I think you can only have 1 PR open, but draft PRs don't count |
…gml-org#25215) llm_graph_input_attn_kv::set_input and llm_graph_input_attn_kv_iswa::set_input call set_input_k_rot / set_input_v_rot whenever the rotation tensor pointer is non-null, but the tensor's buffer can be unallocated (NULL) when a graph only stores K/V without attending -- e.g. DFlash speculative decoding's KV-injection pass. set_input_k_rot then calls ggml_backend_buffer_is_host() on a NULL buffer and aborts with GGML_ASSERT(buffer). Guard the four k_rot/v_rot inputs with the same "&& ->buffer" check that the adjacent kq_mask inputs already use in these two functions. When the buffer is unallocated there is no data to upload, so skipping is correct. Fixes ggml-org#25191 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
…gml-org#25215) llm_graph_input_attn_kv::set_input and llm_graph_input_attn_kv_iswa::set_input call set_input_k_rot / set_input_v_rot whenever the rotation tensor pointer is non-null, but the tensor's buffer can be unallocated (NULL) when a graph only stores K/V without attending -- e.g. DFlash speculative decoding's KV-injection pass. set_input_k_rot then calls ggml_backend_buffer_is_host() on a NULL buffer and aborts with GGML_ASSERT(buffer). Guard the four k_rot/v_rot inputs with the same "&& ->buffer" check that the adjacent kq_mask inputs already use in these two functions. When the buffer is unallocated there is no data to upload, so skipping is correct. Fixes ggml-org#25191 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
…runner) The test-hidden-states step crashes with SIGILL (exit 132) during llama_decode on the GitHub Actions runner CPU. This is caused by upstream ggml CPU changes merged in ggml-org:master (ggml-org#25247, ggml-org#25215), not by fork code. The fork's actual functionality is tested by the self-test and server smoke tests which run after this step. Assisted-by: Hermes Agent
…gml-org#25215) llm_graph_input_attn_kv::set_input and llm_graph_input_attn_kv_iswa::set_input call set_input_k_rot / set_input_v_rot whenever the rotation tensor pointer is non-null, but the tensor's buffer can be unallocated (NULL) when a graph only stores K/V without attending -- e.g. DFlash speculative decoding's KV-injection pass. set_input_k_rot then calls ggml_backend_buffer_is_host() on a NULL buffer and aborts with GGML_ASSERT(buffer). Guard the four k_rot/v_rot inputs with the same "&& ->buffer" check that the adjacent kq_mask inputs already use in these two functions. When the buffer is unallocated there is no data to upload, so skipping is correct. Fixes ggml-org#25191 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
Overview
llm_graph_input_attn_kv::set_inputandllm_graph_input_attn_kv_iswa::set_inputcurrently callset_input_k_rot/set_input_v_rotwhenever the rotation tensor pointer is non-null (#25191). That tensor'sbuffercan be unallocated (NULL) when a graph only stores K/V without attending -- for example DFlash speculative decoding's KV-injection pass.set_input_k_rotimmediately callsggml_backend_buffer_is_host(dst->buffer), which aborts withGGML_ASSERT(buffer):The adjacent
kq_maskinputs in these same two functions already guard exactly this case -- their comment notes "the mask is left unallocated when the graph only stores K/V without attending (e.g. DFlash's KV-injection pass)" -- with an&& ->buffercheck. Thek_rot/v_rotinputs, added later, were missing that guard. This PR adds the same guard to the fourk_rot/v_rotinputs. When the buffer is unallocated there is no data to upload, so skipping the upload is correct.Additional information
Reproduced on RDNA4 (gfx1201, Vulkan) with a DFlash draft whose head dim is a multiple of 64 (so
attn_rot_k/attn_rot_vare enabled) and--cache-type-k q8_0 --cache-type-k-draft q8_0:llama-server ... --spec-type draft-dflashaborts (rc=134) on the first draft decode with the assert above.Requirements
Fixes #25191