Repository navigation
metal: add the missing f32 x bf16 mul_mv variants - #28741
Conversation
github-actions
Bot
added
ggml
pcuenca
left a comment
pcuenca
left a comment
There was a problem hiding this comment.
Tested locally. Thanks a lot @ServeurpersoCom!
|
We'll test --outtype f16. If the quality matches, the fix is good but not urgent, since we'll publish both mmproj formats. We’re taking a belt-and-suspenders approach. this fix OK also, because in vocoders, there are quite a few tensors/models that require the dynamic range provided by BF16. |
nikwen
left a comment
nikwen
left a comment
There was a problem hiding this comment.
Should we add tests to test-backend-ops?
As far as I can tell, we have BF16 x F32 tests in there, but no F32 x BF16 tests yet.
Good catch, F32 x BF16 is never exercised. The test alone would not help though: the CPU backend refuses that combination, so the case would be skipped instead of compared. The CPU side has to land first, then the test makes sense. |
ggml_conv_1d_dw builds its im2col as f32 when the kernel is bf16, then multiplies the two, so a depthwise convolution over bf16 weights asks for kernel_mul_mv_f32_bf16, which was never instantiated. The base, the _4 and the _short families are filled in next to their bf16 neighbours, inside the same runtime guard, so a device without bf16 support is unaffected.
31bea06 to
260f60b
Compare
|
Rebase only |
ggml_conv_1d_dw builds its im2col as f32 when the kernel is bf16, then multiplies the two, so a depthwise convolution over bf16 weights asks for kernel_mul_mv_f32_bf16, which was never instantiated. The base, the _4 and the _short families are filled in next to their bf16 neighbours, inside the same runtime guard, so a device without bf16 support is unaffected.
ggml_conv_1d_dw builds its im2col as f32 when the kernel is bf16, then multiplies the two, so a depthwise convolution over bf16 weights asks for kernel_mul_mv_f32_bf16, which was never instantiated. The base, the _4 and the _short families are filled in next to their bf16 neighbours, inside the same runtime guard, so a device without bf16 support is unaffected. (cherry picked from commit 9575389)
4 participants
Overview
On macOS, any depthwise 1D convolution over bf16 weights aborts with:
"kernel not found in any metal library: kernel_mul_mv_f32_bf16_short"
ggml_conv_1d_dw builds its im2col as f32 when the kernel is bf16, then calls mul_mat(im2col, kernel), which puts f32 in src0 and bf16 in src1. That combination is never instantiated: the library has f32_f32, f16_f32, f16_f16, bf16_f32 and bf16_bf16, but not f32_bf16. Other convolution paths do not hit it because conv_2d keeps its im2col in f16 whatever the kernel type.
Additional information
I paid attention to the guard because I have had that exact point raised on a previous PR, the bf16 kernels must stay behind GGML_METAL_HAS_BF16. The three variants are added next to their bf16 neighbours, inside the existing
#if defined(GGML_METAL_HAS_BF16)block. That macro is injected at library compile time only when the device reports bf16 support, so devices without it are unaffected.Tested on macOS by @pcuenca, who hit the original abort and confirms it is fixed.
Requirements