Repository navigation
vulkan : honor FP32 source precision in MUL_MAT_ID - #30105
linuxid10t wants to merge 2 commits into
Conversation
| const bool f16_f32_kernel = src1->type == GGML_TYPE_F32; | ||
| bool quantize_y = ctx->device->integer_dot_product && src1->type == GGML_TYPE_F32 && ggml_is_contiguous(src1) && !y_non_contig && (ne11 * ne10) % 4 == 0 && ggml_vk_should_use_mmvq(ctx->device, ne01, ne12, ne10, src0->type); | ||
| // Q8_1 stores its scale and sum in FP16, which can overflow for large activations. | ||
| bool quantize_y = ggml_get_op_params_i32(dst, 3) != GGML_PREC_F32 && ctx->device->integer_dot_product && src1->type == GGML_TYPE_F32 && ggml_is_contiguous(src1) && !y_non_contig && (ne11 * ne10) % 4 == 0 && ggml_vk_should_use_mmvq(ctx->device, ne01, ne12, ne10, src0->type); |
There was a problem hiding this comment.
I think 3 is correct for src1, but I had to dig through ggml.c to check that. @ggerganov what do you think about adding functions like ggml_prec_get_src/ggml_prec_get_acc?
I'm not sure "!= F32" is the right check. Maybe it should be "== Q8 || == Q4 || == DEFAULT"? It's not clear to me what is the right way to express these checks when additional precs can be added in the future, and not really knowing how models generally set the prec, and no documentation of what DEFAULT means.
There was a problem hiding this comment.
adding functions like ggml_prec_get_src/ggml_prec_get_acc?
Yes, we should add those.
About the check - the future-proof way is to use a switch and handle every single case of the enum. If a new value is added to the enum it will create a compile warning.
The GGML_PREC_DEFAULT is deprecated and will be removed in #29801 . It is replaced by GGML_PREC_UNDEFINED which I think should be more clear - the user code didn't provide any information for the src1 data. Atm there is no common fallback in this case because generally each backend does different things. In the future, we can consolidate on a common logic.
|
I have a prototype branch supporting the precision properly, I just haven't gotten around to cleaning it up enough to open a PR yet. |
|
If there is a better more thorough fix that comes up I will close the pull request. It also seems like this issue also similarly affects both the CUDA and ROCm back ends. |
Overview
This fix was initiated to fix Mistral Small 4's performance. It results in an around 10x speedup using my rig with one Radeon AI Pro 9700. There was previous discussion about this issue in PR 29029 and PR 29911. PR 29029 broke the performance for this particular model, but there was discussion about a change that would be PRed in a couple of weeks, but it hasn't yet appeared. This change explicitly avoids the issues of PR 29911 that caused it to be rejected. The change keeps activations as FP32 and adds a check for it to prevent unsafe execution in FP16 as well as a check to prevent it from running on non-FP32 hardware. It now selects the correct, safe FP32 vector kernel. Documentation has also been updated. This is my first PR for this project, so if I am in the wrong let me know.
Additional information
Benchmark Before:
ggml_vulkan: 0 = AMD Radeon AI PRO R9700 (RADV GFX1201) (radv) | uma: 0 | fp16: dot2 | bf16: 1 | fp4: 0 | warp size: 64 | shared memory: 65536 | int dot: 1 | matrix cores: KHR_coopmat
Benchmark After:
ggml_vulkan: 0 = AMD Radeon AI PRO R9700 (RADV GFX1201) (radv) | uma: 0 | fp16: dot2 | bf16: 1 | fp4: 0 | warp size: 64 | shared memory: 65536 | int dot: 1 | matrix cores: KHR_coopmat
Requirements
This fix is Vulkan only.