ggml: uniformize im2col dst_type for all conv ops

[Juste-Leo2]

Overview

This PR adjusts the im2col output type in all convolution operations that use it. Instead of always forcing F16, we keep F16 only when the weight is F16, and use F32 for everything else (BF16, F32, quantized types).

Additional information

This change was discovered while working on the Zaya model, which uses ggml_conv_1d_grouped (#22833). This operation goes through im2col, and the old code forced F16, which caused precision loss with BF16 weights or even crashes with quantized weights. The ggml_conv2d and ggml_conv3d operations had a similar issue, as they passed the weight type directly to im2col without checking.
Related to PR #23112 (Zaya).

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: YES
  • The proposed changes here indirectly follow from previous Zaya-related PRs
  • AI was used to translate this PR into English and improve readability

[Juste-Leo2]

CC @pwilkin in case you'd like to take a look :)

[CISC]

On what backend did it crash on quantized weights?

[Juste-Leo2]

On what backend did it crash on quantized weights?

It crashed on the CPU and Vulkan backends (CUDA was unaffected). I had previously done a temporary workaround (here), but forcing F16 ended up breaking BF16.

[Juste-Leo2]

CC @ngxson, I need this PR to move forward with the rest of the Zaya development. Since you work a lot on vision models and seemed interested in the Zaya draft, I thought it made perfect sense to ping you as a reviewer. Especially since this touches im2col and convolutions, which are critical points for vision operations. Let me know what you think!

[Juste-Leo2]

I just rebased the branch on the latest master, as suggested in the contributing guidelines. Since it's been 3 weeks, I figured this PR might have gotten lost in the notification pile :)

If @ggerganov , @pwilkin , or @ngxson could validate the PR, it would really unblock me for the rest of the Zaya implementation. I need this PR to be merged so I can add the Q8 test for ggml_conv_1d_grouped. Thanks!

[Juste-Leo2]

All instances have been updated to follow this casting logic!

I know that modifying im2col has an impact on vision models, so we definitely need to be careful with it. Please don't hesitate to let me know if any other adjustments are necessary. Thanks again for taking the time to review!

[Juste-Leo2]

@ggerganov Quick ping on this PR! Let me know if the updated im2col casting logic looks good to merge :)

CI fails

[ngxson]

This breaks the multimodal test on CI, so surely will also break existing vision models

[Juste-Leo2]

This breaks the multimodal test on CI, so surely will also break existing vision models

@ngxson You're right, I assume there might be other similar issues in the pipeline due to this change. I'll track them down and push a comprehensive fix as soon as I'm back at my PC (I'm currently traveling and on mobile). Thanks for pointing this out!

[Juste-Leo2]

Ok, after investigating with opencode, I think I've figured out the issue.

The latest change modifies the selection rule for the im2col output type : now, for all weight types except BF16, the im2col output is always F16, regardless of the kernel type. The im2col_f16 function (and its 3D version) had a GGML_ASSERT requiring the kernel (src0) to be F16, however, it is never actually read during the operation. Same thing for nb00, it is checked for F16 but never used. I therefore removed these 3 lines and added a new test with an F32 kernel + F16 output, which reproduces the CI error. Normally, this fix should also work for quantized kernels.

• Before fix: before_fix.txt
• After fix: after_fix.txt

Normally, the other backends should work, they don't have any blocking GGML_ASSERT

The error reproduced locally is identical to the one in the CI. After the fix, everything passes. @ngxson could you re-run the Server test to check if there are any other issues?

[Juste-Leo2]

@ngxson Friendly ping on this! CI tests are passing with the latest fix (the editorConfig failure is unrelated to this PR). Let me know if the changes look good to you now so we can move forward with the merge :)

[pwilkin]

@ngxson needs your approval.

[Juste-Leo2]

@ggerganov Quick ping on this! The CI issue has been fixed and @pwilkin approved the changes last Friday. I imagine @ngxson might be quite busy right now, so since his review status is still blocking the merge, could you perhaps take a look and merge it if it's good to go? Getting this merged would allow me to rebase #22833 (ggml_conv_1d_grouped) and add the Q8 and BF16 tests. Thanks!

[ggerganov]

Please rebase on the latest master

[Juste-Leo2]

@ggerganov Done! Rebased on the latest master.

[Juste-Leo2]

Happy to be version b10000, Congratulations on the llama.cpp project ! 🎉