ggml : add support for CPU f16->f16 GGML_OP_SET_ROWS#25344
Conversation
| memcpy( | ||
| ((char *) dst->data + i1*nb1 + i02*nb2 + i03*nb3), | ||
| ((char *) src0->data + i*nb01 + i02*nb02 + i03*nb03), | ||
| rs); | ||
| } |
There was a problem hiding this comment.
You can use the same function as ggml_compute_forward_set_rows_f16, just pass in the template param as type
There was a problem hiding this comment.
Sorry, no idea what do you mean, could you elaborate?
There was a problem hiding this comment.
I mean use the same function as the f32 version, just use a template instead of adding a new function
There was a problem hiding this comment.
I mean use the same function as the f32 version, just use a template instead of adding a new function
You mean to add src0 type template argument in ggml_compute_forward_set_rows_f32() (suffix would be extra confusing, so I'd have to rename it to ggml_compute_forward_set_rows_impl()) and switch logic inside with if constexpr instead of using two different implementations?
Good idea, I don't like touching existing working code but will try.
There was a problem hiding this comment.
Yes, it's okay to change because the bodies are identical except the from_float vs memcpy for the copy
| assert(ne0 == nc); | ||
| assert(ne2 == ne02); | ||
| assert(ne3 == ne03); | ||
| assert(src0->type == GGML_TYPE_F16); | ||
| assert(dst->type == GGML_TYPE_F16); | ||
| assert(ne02 % ne11 == 0); | ||
| assert(ne03 % ne12 == 0); |
There was a problem hiding this comment.
These should be GGML_ASSERT, I think otherwise they will not fire in release builds
There was a problem hiding this comment.
Shall I update them in ggml_compute_forward_set_rows_f32() too?
There was a problem hiding this comment.
If you combining the two functions then yes
There was a problem hiding this comment.
To remind, the logic for when to use assert and GGML_ASSERT is that the latter is typically used when we have to assert some user-provided values and the former is used to clarify the assumptions that would be always true unless there is a bug in the internal logic.
Technically, at this point we should have already GGML_ASSERTed (typically in the ggml.c implementation of ggml_set_rows) the conditions required for the op to make sense and here we just need to assert. GGML_ASSERT here would be needed only for things that are specific to the CPU implementation. So if I am reading correctly:
assert(ne0 == nc);
assert(ne2 == ne02);
assert(ne3 == ne03);
GGML_ASSERT(src0->type == GGML_TYPE_F16);
GGML_ASSERT(dst->type == GGML_TYPE_F16);
assert(ne02 % ne11 == 0);
assert(ne03 % ne12 == 0);There was a problem hiding this comment.
Why not just use GGML ASSERT everywhere? If the performance is the same then I would prefer that as it's easier to remember.
There was a problem hiding this comment.
The asserts are pre-processed away in release builds, so the performance should be better as we do fewer checks. Though likely it's something that's hard to measure in practice.
We are not very strict about this convention, so if you prefer you can use GGML_ASSERT. It's something that can be refined in refactoring passes.
There was a problem hiding this comment.
Yes a potential footgun with assert is exactly what you said about the release builds. The debug build path is not exercised and something like this which should be ideally crash the process as it invalidates an invariant just goes through.
llama.cpp/src/llama-kv-cache.cpp
Lines 2547 to 2548 in e1a7a5a
If a branch is never taken except I would expect these GGML_ASSERTs to be 0-cost on modern processors which execute instructions speculatively
| } break; | ||
| case GGML_TYPE_F16: | ||
| { | ||
| if (src1->type == GGML_TYPE_I64) { |
There was a problem hiding this comment.
We also have the check the dst type so that it's f16. Or perhaps check src->type == dst->type
…ward_set_rows_f16() into ggml_compute_forward_set_rows_impl()
|
Looks like the Vulkan backend needs some fixes in |
Overview
This PR adds support for CPU
GGML_OP_SET_ROWSwheresrc0type is f16 anddsttype is also f16. This will come in handy in models based on DeepSeek Sparse Attention (DeepSeek V3.2, V4, GLM 5.0, 5.1, 5.2) where we have to "sparsify" KQ mask based on lightning indexer indices. With f16 support inGGML_OP_SET_ROWSit will be possible to use f16 FA KQ masks without any extra casts, this will reduce memory usage and simplify code.Additional information
This PR adds only CPU support with
ggml_compute_forward_set_rows_f16()implementation based on existingggml_compute_forward_set_rows_f32()withfrom_floatcalls replaced with a simplememcpy().Backend OPs tests for
ggml_set_rows()were also updated to include extra src type parameter and f16->f16 test cases.CUDA implementation will follow next.
CC @am17an
Requirements