Repository navigation
ggml : allow MUL_MAT_ID -1 index to skip the computation - #26631
Conversation
ggerganov
left a comment
There was a problem hiding this comment.
The Metal changes are good.
|
I think CUDA / vulkan / webgpu changes are still not correct though, but still keeping this PR if backend maintainers are interested in fixing it (you can push to another branch then I will cherry-pick it) |
| (uint32_t)(get_misalign_bytes(ctx, ids) / ggml_type_size(ids->type)) }; | ||
| (uint32_t)(get_misalign_bytes(ctx, ids) / ggml_type_size(ids->type)), | ||
| (uint32_t)ne01, | ||
| (uint32_t)ne01, |
There was a problem hiding this comment.
Should these last two be strides? But this might work if the output is guaranteed contiguous
|
I think the vulkan code is probably fine as-is. I had one small suggestion and one non-issue that I resolved. |
|
@ngxson I pushed a fix for the webgpu backend to the following branch, so can you cherry-pick the latest commit? I confirmed that https://github.com/yomaytk/llama.cpp/tree/pr-26631-fix-webgpu The commit ensures that all threads reach the workgroup barriers in |
max-krasnyansky
left a comment
There was a problem hiding this comment.
For hexagon we're going to need to replace memset(0) with hvx_splat_f32(0) for better performance, but we can do that in a follow up PR once the rest of the stuff is merged.
…d_vec (cherry picked from commit d8a4b9e)
|
@ggml-org/ggml-cuda can one of you review this PR? Thank you! |
|
@ngxson arthw reviewed it for sycl, I don't need to review. I just noticed this PR was sitting here for a while so wanted to push it along |
|
Vulkan changes are fine |
| const dim3 num_blocks(n_expert_used, n_tokens, 1); | ||
| mm_ids_zero_dst<<<num_blocks, CUDA_MM_IDS_ZERO_BLOCK_SIZE, 0, stream>>>(ids, dst, ne0, si1, s1, s2); |
There was a problem hiding this comment.
n_tokens here is blockIdx.y which will overflow for n_tokens > 65535.
|
After the #30373 changes, the Metal patch here will likely need to be adjusted a bit. |
|
@ggerganov @JohannesGaessler @am17an could you please have a look? I'm waiting for final approval for metal and CUDA |
|
OK so as @JohannesGaessler requested, I excluded CUDA from this PR. For I will merge this PR once the CI passes. Won't ask for another round of review, otherwise it will take forever 😓 |
| initialize_tensors(ctx_weights.get()); | ||
| } | ||
|
|
||
| // [TAG_MUL_MAT_ID_SKIP] @ngxson : remove this once CUDA implements mul_mat_id with -1 = skipped |
There was a problem hiding this comment.
Add a TODO tag so we keep track of this in the https://github.com/ggml-org/llama.cpp-dev/blob/master/README-todo-counts.md
Overview
Sorry for the big PR, but unfortunately this change cannot be gated by a graph build-time condition, so I need to add it to all backends at once. Important to note: code is 100% AI-generated.
This change allow
mul_mat_idto read-1index as a sentiment for "skip computation". This allow implement an "expert mask" similar to pytorch impl of MoE.It's useful for these 2 models:
mul_mat_idafter this PR, one masked small exp and one masked big expDetails about masking logic
The masking impl wll be like this:
Generalized formula for masking exp >= N:
Additional info
Most backends are covered, but I only tested on Metal.
Some are real skip (computation is skipped), and some is fake mul mat with 0
Important: CUDA is not included in this PR, see comments below
ggml-cpu.cmul_mat_idmatrix_rows, dst row memsetrepack.cppspacemit/ime.cppkernel_mul_mm_id(batch >= 32)map0marks the slot 0xFFFF so no expert claims it,kernel_mul_mm_id_zerowrites 0kernel_mul_mv_id(batch < 32)mul_mm/mul_mmq/mul_mm_cm2(batch)count_experts+load_row_ids,count_expertsalso writes 0 to the dst rowmul_mat_vec(bs=1)get_offsetsclamps the expert to 0,reduce_resultoverwrites with 0k_zero_dst_rowskernel writes 0ne12 == 1per-slot pathggml_sycl_mul_matnot called,memsetinsteadgemmMoE (sorted, batch)kernel_moe_histogram/kernel_moe_scatter,kernel_moe_zero_dstwrites 0gemv_moe_*andmul_mv_id_*kernel_moe_zero_dstwrites 0mul_mat_id(batch)mul_mat_id_gather, same shader writes 0 to the dst rowmul_mat_id_vec(bs=1)gathered_count_ids, workgroup 0 writes 0, extra workgroups returnop_matmul_id(HMX + HVX)scan_expert_ids/hvx_mv_idalready skipped-1,zero_skipped_dst_rowswrites 0mul_mat_idmul_mat_id_*kernelsggml_cann_mul_mat_id_fp/_quantaclnn IndexSelectwith the ids verbatim; a negative index is invalidtranslate_mul_mat_idov::op::Gather, where a negative index means "from the end" -> silently picks the wrong expertRequirements