Generalize `quantize_fns` for simpler FP16 handling by sw · Pull Request #1237 · ggml-org/llama.cpp

sw · 2023-04-29T18:13:58Z

This is hopefully in line with the goal of refactoring in the May roadmap.

In my opinion, quantize_fns is too specific: what matters is not whether a type is quantized, but whether it requires a row-wise conversion, either to vec_dot_type for the vector dot-product, or to float for some other operations.

By making this more generic, some specific code paths for FP16 can be eliminated, most importantly ggml_compute_forward_mul_mat_f16_f32.

This is not finished; CUDA and Accelerate may not work, and also the work buffer allocation may be wrong.

ggerganov · 2023-04-29T18:44:22Z

This is hopefully in line with the goal of refactoring in the May roadmap.

Yes, this looks great

slaren · 2023-04-29T19:11:01Z

It would be great if mul_mat_f32 could also be unified like this, maybe by making dummy from/to_float functions. I would be happy to move all the cuBLAS mul mat handling to a single mul mat function to simplify this change.

sw · 2023-04-30T13:23:35Z

Tried to get the BLAS stuff right as best as I could, at this point I need the help of someone with Accelerate and cuBLAS hardware and experience.

slaren · 2023-04-30T18:41:58Z

@sw I have moved all the cuBLAS mat mul code to a single function that should make your changes here easier, but it depends on #1233. Once that is merged, I will open a PR.

ggerganov · 2023-05-01T07:31:10Z

The Accelerate path works - tested the perplexity tool for quantized and F16 models

slaren · 2023-05-01T11:54:45Z

cuBLAS has different paths depending on the mat mul type, so this does not look right currently. #1259 should make this much easier for you since it coalesces all cuBLAS mat mul code in a function call to ggml_cuda_mul_mat such as:

#if defined(GGML_USE_CUBLAS)
    if (ggml_cuda_can_mul_mat(src0, src1, dst)) {
        if (params->ith == 0 && params->type == GGML_TASK_COMPUTE) {
            ggml_cuda_mul_mat(src0, src1, dst, params->wdata, params->wsize);
        }
        return;
    }
#endif

Incidentally, this also means that it is possible to fall back to other BLAS if cuBLAS cannot perform a mat mul, but currently that is never the case, but opens the door to conditional usage of CUDA.

goerch · 2023-06-27T20:40:06Z

@sw : now that this is high priority and on the roadmap, do you intend to update this patch? Otherwise I'll consider to look into it once ggml-org/ggml#309 is resolved.

But my means to check non-CPU stuff will be severely limited, too.

sw · 2023-06-30T12:26:05Z

I've rebased the branch onto latest master. I'm not particularly happy with the name type_handling though, any suggestions?

slaren · 2023-06-30T12:32:10Z

How about something like type_traits. In the future, it could be expanded to include other details that currently are spread around everywhere, such as the sizes or the name.

goerch · 2023-07-02T18:03:48Z

I've rebased the branch onto latest master. I'm not particularly happy with the name type_handling though, any suggestions?

Sorry, I missed that completely. In my PR against ggml I simply keep the original names until @ggerganov objects. But on to a more important question: I just checked and it seems parts of our changes are not covered by the existing tests. How do you validate your changes?

Edit: it seems we are going with type_traits...

ggerganov · 2023-07-02T18:35:52Z

@goerch
Yes, I think type_traits is a good suggestion.
I think we should proceed and merge this PR first as it deduplicates the forward matrix multiplication code which is nice.

@sw
Let us know when this is ready for review - it seems in a good shape to me.

sw · 2023-07-02T19:38:29Z

Rebased and renamed as suggested. I'll mark this as ready now, but as mentioned before I don't have a wide range of hardware to test the non-Intel stuff.

ggerganov · 2023-07-03T19:38:20Z

Thanks - will look to merge tomorrow

slaren · 2023-07-03T22:33:51Z

                        if (ggml_cuda_can_mul_mat(node->src0, node->src1, node)) {
                            node->n_tasks = 1; // TODO: this actually is doing nothing
                                                //       the threads are still spinning
+                            cur = ggml_cuda_mul_mat_get_wsize(node->src0, node->src1, node);


This line can be removed as it is no longer necessary. That should fix the cuBLAS build errors.
The function and a few others should also be removed from ggml-cuda.h, but that's a different issue.

You're right, I didn't notice that I introduced a regression there because those builds take so much time. Removed the line and it builds fine but I can't test it on my machine. The failure on macOS on the other hand was already on master but seems to have been fixed in the meantime.

ggerganov · 2023-07-04T18:55:59Z

I'll rebase now and run a few tests before merging

ggerganov · 2023-07-04T19:47:00Z

Strange - no idea why the MacOS CI is failing. It works locally both on x86 and arm

ggerganov

Will merge this first thing tomorrow and give it some more time for testing and hopefully resolving the CI

mqy · 2023-07-04T20:10:18Z

Strange - no idea why the MacOS CI is failing. It works locally both on x86 and arm

If I remember correctly, I've experienced similar situation when bench mul_mat for f16 (the rope case) -- only macOS-latest-cmake fail with Illegal. I'll pull this branch and have a try.

[EDIT] macOS 13.4.1, intel CPU.

cmake .. -DLLAMA_AVX2=OFF  <--- NOTE macOS-latest-make does not disable AVX2
cmake --build . --config Release
./bin/test-quantize-fns    <-- OK
./bin/test-quantize-perf   <-- OK

The OS of macos-latest-cmake:

Operating System
  macOS
  12.6.7
  21G651

ggerganov · 2023-07-05T17:27:30Z

+                        if (node->src1->type != vec_dot_type) {
+                            cur = GGML_TYPE_SIZE[vec_dot_type]*ggml_nelements(node->src1)/GGML_BLCK_SIZE[vec_dot_type];
                        } else {
                            GGML_ASSERT(false);


We hit this assert when both src0 and src1 are F32

Pushed the following fix for now: ec326d3

Probably would be best to add a unit test.
Will make an issue

…1237) * Generalize quantize_fns for simpler FP16 handling * Remove call to ggml_cuda_mul_mat_get_wsize * ci : disable FMA for mac os actions --------- Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>

sw force-pushed the type-handling branch from 508a181 to 7a633bf Compare April 30, 2023 13:21

sw added help wanted Needs help from the community refactoring Refactoring labels Apr 30, 2023

sw force-pushed the type-handling branch from 7a633bf to 7d58c84 Compare May 1, 2023 17:29

sw force-pushed the type-handling branch from 7d58c84 to 86503a5 Compare May 12, 2023 11:22

sw force-pushed the type-handling branch 2 times, most recently from 62c2f37 to 8cb93d6 Compare June 9, 2023 19:16

ggerganov added the high priority Very important issue label Jun 10, 2023

ggerganov mentioned this pull request Jun 25, 2023

ggml : generalize quantize_fns for simpler FP16 handling ggml-org/ggml#286

Closed

sw force-pushed the type-handling branch from 8cb93d6 to 3770f4f Compare June 30, 2023 12:21

ggerganov mentioned this pull request Jul 2, 2023

[WIP]: Fix #286 ggml-org/ggml#317

Closed

Generalize quantize_fns for simpler FP16 handling

f9c585f

sw force-pushed the type-handling branch from 3770f4f to f9c585f Compare July 2, 2023 19:34

sw marked this pull request as ready for review July 2, 2023 19:38

sw requested review from ggerganov and slaren July 3, 2023 19:23

slaren reviewed Jul 3, 2023

View reviewed changes

Remove call to ggml_cuda_mul_mat_get_wsize

81f28f2

Merge branch 'master' into HEAD

8e9af80

ggerganov approved these changes Jul 4, 2023

View reviewed changes

ci : disable FMA for mac os actions

745e89e

ggerganov merged commit 1b107b8 into ggml-org:master Jul 5, 2023

ggerganov reviewed Jul 5, 2023

View reviewed changes

ggerganov added a commit that referenced this pull request Jul 5, 2023

ggml : fix bug introduced in #1237

ec326d3

sw deleted the type-handling branch July 5, 2023 18:09

sw mentioned this pull request Jul 10, 2023

Segmentation Fault Error "not enough space in the context's memory pool" #52

Closed

goerch mentioned this pull request Jul 11, 2023

ggml : improve CI + add more tests ggml-org/ggml#295

Open

1 task

Bearsaerker mentioned this pull request Mar 12, 2025

Eval bug: Gemma 3 extremly slow prompt processing when using quantized kv cache. #12352

Closed

Seunghhon pushed a commit to Seunghhon/llama.cpp that referenced this pull request Apr 26, 2026

ggml : fix bug introduced in ggml-org#1237

2e3fcb7

phuongncn pushed a commit to phuongncn/llama.cpp-gx10-dgx-sparks-deepseekv4 that referenced this pull request Apr 28, 2026

ggml : fix bug introduced in ggml-org#1237

bd32bac

phuongncn pushed a commit to phuongncn/llama.cpp-gx10-dgx-sparks-deepseekv4 that referenced this pull request Apr 28, 2026

Fix ggml-org#1237 (ggml-org#1238)

1ec12b8

ljubomirj pushed a commit to ljubomirj/llama.cpp that referenced this pull request May 6, 2026

ggml : fix bug introduced in ggml-org#1237

2f3fd6f

AlexiAlp pushed a commit to minghaop/llama.cpp that referenced this pull request Jun 2, 2026

ggml : fix bug introduced in ggml-org#1237

0186e47

AlexiAlp pushed a commit to minghaop/llama.cpp that referenced this pull request Jun 2, 2026

ggml : fix bug introduced in ggml-org#1237

7cd034f

Conversation

sw commented Apr 29, 2023 • edited by ggerganov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggerganov commented Apr 29, 2023

Uh oh!

slaren commented Apr 29, 2023

Uh oh!

sw commented Apr 30, 2023

Uh oh!

slaren commented Apr 30, 2023

Uh oh!

ggerganov commented May 1, 2023

Uh oh!

slaren commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goerch commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sw commented Jun 30, 2023

Uh oh!

slaren commented Jun 30, 2023

Uh oh!

goerch commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggerganov commented Jul 2, 2023

Uh oh!

sw commented Jul 2, 2023

Uh oh!

ggerganov commented Jul 3, 2023

Uh oh!

slaren Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

sw Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

ggerganov commented Jul 4, 2023

Uh oh!

ggerganov commented Jul 4, 2023

Uh oh!

ggerganov left a comment

Choose a reason for hiding this comment

Uh oh!

mqy commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggerganov Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

ggerganov Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sw commented Apr 29, 2023 •

edited by ggerganov

Loading

slaren commented May 1, 2023 •

edited

Loading

goerch commented Jun 27, 2023 •

edited

Loading

goerch commented Jul 2, 2023 •

edited

Loading

mqy commented Jul 4, 2023 •

edited

Loading