Repository navigation
ggml-cpu: add F16 input to the FWHT - #27779
Conversation
|
Hi @bri-prism, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
a30afab to
8e596f9
Compare
|
Thanks. I closed the other PR, #26883, so this is now my only open PR. It was obsolete: master already sets I also reduced the scope of this PR. It was 5 files across two backends. It is now 3 files and touches |
|
Hi @ggerganov — gentle ping when you have a moment: would you mind taking a look at this PR? Thank you! |
The CPU FWHT accepts F32 input only. This change makes the source type a template parameter. The CPU path now accepts F16 input and F32 input. The CPU MUL_MAT reference now converts an F16 src1 to F32. It does this when the caller sets the Hadamard hint. No backend has an F16 FWHT kernel yet. The test cases come with the backend changes that add one.
|
Gentle ping on this one. The PR checker's two items are addressed: this is my only open PR, and the diff is three files under So far only the labeler workflow has run, so I think the build and test workflows still need maintainer approval for a first-time contributor. Happy to rebase onto current master or narrow the scope further if that would help. |
8e596f9 to
cc3c259
Compare
| } else { | ||
| const ggml_fp16_t * src_f16 = (const ggml_fp16_t *) src1_block; | ||
| float * dst_f32 = (float *) dst_block; | ||
| for (int64_t i = 0; i < n_block; ++i) { | ||
| dst_f32[i] = GGML_CPU_FP16_TO_FP32(src_f16[i]); | ||
| } |
There was a problem hiding this comment.
This is only correct if vec_dot_type == GGML_TYPE_F32. Needs asserts.
There was a problem hiding this comment.
Added in b478d66. The F16 branch writes plain floats into wdata, so it needs vec_dot_type == GGML_TYPE_F32; the assert now states that explicitly alongside the existing type check.
| for (int64_t i = 0; i < n_block; ++i) { | ||
| dst_f32[i] = GGML_CPU_FP16_TO_FP32(src_f16[i]); | ||
| } |
There was a problem hiding this comment.
There is a faster implementation: ggml_cpu_fp16_to_fp32
There was a problem hiding this comment.
Switched to ggml_cpu_fp16_to_fp32 in b478d66. The F32 path still uses from_float unchanged.
Address review feedback. The F16 branch writes plain floats into wdata, which is only correct when vec_dot_type is F32. That invariant held because supports_op only accepts an F16 src1 for the Hadamard hint with F32 src0 and dst, but nothing enforced it. Assert it next to the existing src1 type check so widening supports_op cannot silently break the write. Replace the hand-rolled conversion loop with ggml_cpu_fp16_to_fp32.
|
I'm assuming this is needed to get ternary bonsai 2 27b work started? I saw you added metadata to your ggufs that says which weights have a rotation applied. @ggerganov having the ability to apply rotations to the weights in a generic way could benefit every low bit quant for some edit: probably not minor. In their whitepaper they say:
|
|
@Green-Sky Yes, this is for Bonsai-2 and yeah the rotations can help everyone probably :) Next PR is out: #29094 |
* ggml-cpu: add F16 input to the FWHT The CPU FWHT accepts F32 input only. This change makes the source type a template parameter. The CPU path now accepts F16 input and F32 input. The CPU MUL_MAT reference now converts an F16 src1 to F32. It does this when the caller sets the Hadamard hint. No backend has an F16 FWHT kernel yet. The test cases come with the backend changes that add one. * ggml-cpu: assert the F16 FWHT input path, and use the bulk converter Address review feedback. The F16 branch writes plain floats into wdata, which is only correct when vec_dot_type is F32. That invariant held because supports_op only accepts an F16 src1 for the Hadamard hint with F32 src0 and dst, but nothing enforced it. Assert it next to the existing src1 type check so widening supports_op cannot silently break the write. Replace the hand-rolled conversion loop with ggml_cpu_fp16_to_fp32.
Bonsai 2 stores its weights in a rotated basis and needs a runtime Walsh-Hadamard transform to read them. Stock llama.cpp has no such transform (ggml-org/llama.cpp#27779): it rejects PTQ1_0 and PQ2_0 as unknown ggml types outright, and loads the legacy Q2_0 without complaint while emitting gibberish. The fork carries the ternary hybrid-attention kernels, rebases on upstream and publishes the same asset names, so moving the sidecar to it is a repo swap and nothing more. The two older variants keep working there -- verified by loading Ternary Bonsai 27B on the fork build and generating from it. BONSAI_RELEASE_REPO points fetch-sidecar.sh back at ggml-org for anyone who wants a stock build; everything but Bonsai 2 runs on one. The new entry sits after the older two, so an existing install keeps starting the variant it already has: list order decides the fallback. PQ2_0, the same weights in a looser packing, is offered on macOS only. At the pinned tag the ternary kernels cover it under CUDA and Metal but not Vulkan, and the Linux and Windows bundles are Vulkan builds precisely so they run on AMD and Intel with no CUDA install -- shipping it there would mean a second, NVIDIA-only sidecar. It costs 1.26 GB over PTQ1_0 and buys cheaper unpacking, so it is a real choice where the kernels exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* metal: add F16 input to the FWHT The Metal FWHT kernel accepts F32 input only. This change makes the source type a template parameter, so the kernel reads an F16 source directly instead of requiring a converted copy. The F32 instantiations are unchanged. The pipeline name now carries the source type, and supports_op accepts an F16 src1 for the Hadamard hint at the four sizes the kernels cover. Every other F16 src1 path still goes through ggml_metal_supports_mul_mat_op. These are the test cases mentioned in #27779. test-backend-ops on M5 Pro: MUL_MAT_HADAMARD 16/16, MUL_MAT 1265/1265. * metal: ask the same FWHT question in supports_op and the dispatch supports_op admitted an F16 src1 on the type, the hint and the width alone, but the dispatch also requires src1 and dst to be contiguous and the same shape. A Hadamard hinted MUL_MAT that passed the first and failed the second reached the generic path, which has no F32 src0 by F16 src1 kernel, and aborted on a nil pipeline: kernel not found in any metal library: base = 'kernel_mul_mv_f32_f16_4' ggml_metal_encoder_set_pipeline: nil Metal pipeline ggml_metal_use_fwht now holds the whole condition and both callers use it, so they cannot drift apart again. The added test case has src1 and dst of different shapes, which aborted before this change and is declined by the Metal backend after it. * metal: branchless butterfly select in the FWHT simdgroup kernel Review suggestion. Replaces the ternary in the shuffle stages with val2 - val + 2*((lane & i) == 0)*val, which is the same value without the select. Measured on M5 Pro, interleaved A/B, five rounds, first discarded, on a Hadamard matmul with block 512 and 65536 rows so the kernel rather than the launch dominates: 1324.6 us before, 1285.0 us after, a 3.0% gain, and faster in every round. At the shapes already in the perf suite the op runs 1.6 to 3.9 us against a 1.6 us launch floor, so the difference is not visible there. FOR_UNROLL on the same loops was also measured and made no difference, the delta changing sign between rounds, so it is not included. * metal: move the FWHT dispatch predicates to ggml-metal-common Review feedback. ggml_metal_use_fwht and ggml_metal_fwht_supported_size were static inline in ggml-metal-device.h. They now follow the ggml_metal_op_mul_mat_use_mm pattern: declared in ggml-metal-common.h and implemented in ggml-metal-common.cpp, which is already the home for helpers shared between supports_op and the op dispatch. The predicate is named ggml_metal_op_mul_mat_use_fwht to sit alongside the _use_mm pair it parallels. This also fixes the macos-latest-arm64 build. The header needed ggml-impl.h for ggml_get_op_params_i32, but ggml-metal-device.h is reached from tools/tuning through ggml-metal-tuning.h, and that target does not have ggml/src on its include path. ggml-metal-common.cpp already includes ggml-impl.h, so the accessor is used normally there and the header goes back to needing nothing extra. * metal: keep the FWHT size check internal and group the dispatch helpers Applies the patch from the review. ggml_metal_fwht_supported_size becomes static in ggml-metal-common.cpp since nothing outside it needs the size list, which also drops stdint.h from the header again, and ggml_metal_op_mul_mat_use_fwht joins the existing _use_mm declarations under their shared comment instead of carrying its own block. * tests: drop the mismatched-shape Hadamard case I added a case with m != k to cover an abort, but the hint is a promise that src0 is a Hadamard matrix, so src0 is square and dst has the same shape as src1. Every other case in the suite holds to that. The case was not a valid op, and on CPU it compared the FWHT against a real matmul of a non-square src0, which cannot agree. The supports_op and dispatch conditions still come from one predicate, which is what keeps them from disagreeing on contiguity.
* ggml-cpu: add F16 input to the FWHT The CPU FWHT accepts F32 input only. This change makes the source type a template parameter. The CPU path now accepts F16 input and F32 input. The CPU MUL_MAT reference now converts an F16 src1 to F32. It does this when the caller sets the Hadamard hint. No backend has an F16 FWHT kernel yet. The test cases come with the backend changes that add one. * ggml-cpu: assert the F16 FWHT input path, and use the bulk converter Address review feedback. The F16 branch writes plain floats into wdata, which is only correct when vec_dot_type is F32. That invariant held because supports_op only accepts an F16 src1 for the Hadamard hint with F32 src0 and dst, but nothing enforced it. Assert it next to the existing src1 type check so widening supports_op cannot silently break the write. Replace the hand-rolled conversion loop with ggml_cpu_fp16_to_fp32.
* metal: add F16 input to the FWHT The Metal FWHT kernel accepts F32 input only. This change makes the source type a template parameter, so the kernel reads an F16 source directly instead of requiring a converted copy. The F32 instantiations are unchanged. The pipeline name now carries the source type, and supports_op accepts an F16 src1 for the Hadamard hint at the four sizes the kernels cover. Every other F16 src1 path still goes through ggml_metal_supports_mul_mat_op. These are the test cases mentioned in ggml-org#27779. test-backend-ops on M5 Pro: MUL_MAT_HADAMARD 16/16, MUL_MAT 1265/1265. * metal: ask the same FWHT question in supports_op and the dispatch supports_op admitted an F16 src1 on the type, the hint and the width alone, but the dispatch also requires src1 and dst to be contiguous and the same shape. A Hadamard hinted MUL_MAT that passed the first and failed the second reached the generic path, which has no F32 src0 by F16 src1 kernel, and aborted on a nil pipeline: kernel not found in any metal library: base = 'kernel_mul_mv_f32_f16_4' ggml_metal_encoder_set_pipeline: nil Metal pipeline ggml_metal_use_fwht now holds the whole condition and both callers use it, so they cannot drift apart again. The added test case has src1 and dst of different shapes, which aborted before this change and is declined by the Metal backend after it. * metal: branchless butterfly select in the FWHT simdgroup kernel Review suggestion. Replaces the ternary in the shuffle stages with val2 - val + 2*((lane & i) == 0)*val, which is the same value without the select. Measured on M5 Pro, interleaved A/B, five rounds, first discarded, on a Hadamard matmul with block 512 and 65536 rows so the kernel rather than the launch dominates: 1324.6 us before, 1285.0 us after, a 3.0% gain, and faster in every round. At the shapes already in the perf suite the op runs 1.6 to 3.9 us against a 1.6 us launch floor, so the difference is not visible there. FOR_UNROLL on the same loops was also measured and made no difference, the delta changing sign between rounds, so it is not included. * metal: move the FWHT dispatch predicates to ggml-metal-common Review feedback. ggml_metal_use_fwht and ggml_metal_fwht_supported_size were static inline in ggml-metal-device.h. They now follow the ggml_metal_op_mul_mat_use_mm pattern: declared in ggml-metal-common.h and implemented in ggml-metal-common.cpp, which is already the home for helpers shared between supports_op and the op dispatch. The predicate is named ggml_metal_op_mul_mat_use_fwht to sit alongside the _use_mm pair it parallels. This also fixes the macos-latest-arm64 build. The header needed ggml-impl.h for ggml_get_op_params_i32, but ggml-metal-device.h is reached from tools/tuning through ggml-metal-tuning.h, and that target does not have ggml/src on its include path. ggml-metal-common.cpp already includes ggml-impl.h, so the accessor is used normally there and the header goes back to needing nothing extra. * metal: keep the FWHT size check internal and group the dispatch helpers Applies the patch from the review. ggml_metal_fwht_supported_size becomes static in ggml-metal-common.cpp since nothing outside it needs the size list, which also drops stdint.h from the header again, and ggml_metal_op_mul_mat_use_fwht joins the existing _use_mm declarations under their shared comment instead of carrying its own block. * tests: drop the mismatched-shape Hadamard case I added a case with m != k to cover an abort, but the hint is a promise that src0 is a Hadamard matrix, so src0 is square and dst has the same shape as src1. Every other case in the suite holds to that. The case was not a valid op, and on CPU it compared the FWHT against a real matmul of a non-square src0, which cannot agree. The supports_op and dispatch conditions still come from one predicate, which is what keeps them from disagreeing on contiguity.
* ggml-cpu: add F16 input to the FWHT The CPU FWHT accepts F32 input only. This change makes the source type a template parameter. The CPU path now accepts F16 input and F32 input. The CPU MUL_MAT reference now converts an F16 src1 to F32. It does this when the caller sets the Hadamard hint. No backend has an F16 FWHT kernel yet. The test cases come with the backend changes that add one. * ggml-cpu: assert the F16 FWHT input path, and use the bulk converter Address review feedback. The F16 branch writes plain floats into wdata, which is only correct when vec_dot_type is F32. That invariant held because supports_op only accepts an F16 src1 for the Hadamard hint with F32 src0 and dst, but nothing enforced it. Assert it next to the existing src1 type check so widening supports_op cannot silently break the write. Replace the hand-rolled conversion loop with ggml_cpu_fp16_to_fp32.
* metal: add F16 input to the FWHT The Metal FWHT kernel accepts F32 input only. This change makes the source type a template parameter, so the kernel reads an F16 source directly instead of requiring a converted copy. The F32 instantiations are unchanged. The pipeline name now carries the source type, and supports_op accepts an F16 src1 for the Hadamard hint at the four sizes the kernels cover. Every other F16 src1 path still goes through ggml_metal_supports_mul_mat_op. These are the test cases mentioned in ggml-org#27779. test-backend-ops on M5 Pro: MUL_MAT_HADAMARD 16/16, MUL_MAT 1265/1265. * metal: ask the same FWHT question in supports_op and the dispatch supports_op admitted an F16 src1 on the type, the hint and the width alone, but the dispatch also requires src1 and dst to be contiguous and the same shape. A Hadamard hinted MUL_MAT that passed the first and failed the second reached the generic path, which has no F32 src0 by F16 src1 kernel, and aborted on a nil pipeline: kernel not found in any metal library: base = 'kernel_mul_mv_f32_f16_4' ggml_metal_encoder_set_pipeline: nil Metal pipeline ggml_metal_use_fwht now holds the whole condition and both callers use it, so they cannot drift apart again. The added test case has src1 and dst of different shapes, which aborted before this change and is declined by the Metal backend after it. * metal: branchless butterfly select in the FWHT simdgroup kernel Review suggestion. Replaces the ternary in the shuffle stages with val2 - val + 2*((lane & i) == 0)*val, which is the same value without the select. Measured on M5 Pro, interleaved A/B, five rounds, first discarded, on a Hadamard matmul with block 512 and 65536 rows so the kernel rather than the launch dominates: 1324.6 us before, 1285.0 us after, a 3.0% gain, and faster in every round. At the shapes already in the perf suite the op runs 1.6 to 3.9 us against a 1.6 us launch floor, so the difference is not visible there. FOR_UNROLL on the same loops was also measured and made no difference, the delta changing sign between rounds, so it is not included. * metal: move the FWHT dispatch predicates to ggml-metal-common Review feedback. ggml_metal_use_fwht and ggml_metal_fwht_supported_size were static inline in ggml-metal-device.h. They now follow the ggml_metal_op_mul_mat_use_mm pattern: declared in ggml-metal-common.h and implemented in ggml-metal-common.cpp, which is already the home for helpers shared between supports_op and the op dispatch. The predicate is named ggml_metal_op_mul_mat_use_fwht to sit alongside the _use_mm pair it parallels. This also fixes the macos-latest-arm64 build. The header needed ggml-impl.h for ggml_get_op_params_i32, but ggml-metal-device.h is reached from tools/tuning through ggml-metal-tuning.h, and that target does not have ggml/src on its include path. ggml-metal-common.cpp already includes ggml-impl.h, so the accessor is used normally there and the header goes back to needing nothing extra. * metal: keep the FWHT size check internal and group the dispatch helpers Applies the patch from the review. ggml_metal_fwht_supported_size becomes static in ggml-metal-common.cpp since nothing outside it needs the size list, which also drops stdint.h from the header again, and ggml_metal_op_mul_mat_use_fwht joins the existing _use_mm declarations under their shared comment instead of carrying its own block. * tests: drop the mismatched-shape Hadamard case I added a case with m != k to cover an abort, but the hint is a promise that src0 is a Hadamard matrix, so src0 is square and dst has the same shape as src1. Every other case in the suite holds to that. The case was not a valid op, and on CPU it compared the FWHT against a real matmul of a non-square src0, which cannot agree. The supports_op and dispatch conditions still come from one predicate, which is what keeps them from disagreeing on contiguity.
* metal: add F16 input to the FWHT The Metal FWHT kernel accepts F32 input only. This change makes the source type a template parameter, so the kernel reads an F16 source directly instead of requiring a converted copy. The F32 instantiations are unchanged. The pipeline name now carries the source type, and supports_op accepts an F16 src1 for the Hadamard hint at the four sizes the kernels cover. Every other F16 src1 path still goes through ggml_metal_supports_mul_mat_op. These are the test cases mentioned in ggml-org#27779. test-backend-ops on M5 Pro: MUL_MAT_HADAMARD 16/16, MUL_MAT 1265/1265. * metal: ask the same FWHT question in supports_op and the dispatch supports_op admitted an F16 src1 on the type, the hint and the width alone, but the dispatch also requires src1 and dst to be contiguous and the same shape. A Hadamard hinted MUL_MAT that passed the first and failed the second reached the generic path, which has no F32 src0 by F16 src1 kernel, and aborted on a nil pipeline: kernel not found in any metal library: base = 'kernel_mul_mv_f32_f16_4' ggml_metal_encoder_set_pipeline: nil Metal pipeline ggml_metal_use_fwht now holds the whole condition and both callers use it, so they cannot drift apart again. The added test case has src1 and dst of different shapes, which aborted before this change and is declined by the Metal backend after it. * metal: branchless butterfly select in the FWHT simdgroup kernel Review suggestion. Replaces the ternary in the shuffle stages with val2 - val + 2*((lane & i) == 0)*val, which is the same value without the select. Measured on M5 Pro, interleaved A/B, five rounds, first discarded, on a Hadamard matmul with block 512 and 65536 rows so the kernel rather than the launch dominates: 1324.6 us before, 1285.0 us after, a 3.0% gain, and faster in every round. At the shapes already in the perf suite the op runs 1.6 to 3.9 us against a 1.6 us launch floor, so the difference is not visible there. FOR_UNROLL on the same loops was also measured and made no difference, the delta changing sign between rounds, so it is not included. * metal: move the FWHT dispatch predicates to ggml-metal-common Review feedback. ggml_metal_use_fwht and ggml_metal_fwht_supported_size were static inline in ggml-metal-device.h. They now follow the ggml_metal_op_mul_mat_use_mm pattern: declared in ggml-metal-common.h and implemented in ggml-metal-common.cpp, which is already the home for helpers shared between supports_op and the op dispatch. The predicate is named ggml_metal_op_mul_mat_use_fwht to sit alongside the _use_mm pair it parallels. This also fixes the macos-latest-arm64 build. The header needed ggml-impl.h for ggml_get_op_params_i32, but ggml-metal-device.h is reached from tools/tuning through ggml-metal-tuning.h, and that target does not have ggml/src on its include path. ggml-metal-common.cpp already includes ggml-impl.h, so the accessor is used normally there and the header goes back to needing nothing extra. * metal: keep the FWHT size check internal and group the dispatch helpers Applies the patch from the review. ggml_metal_fwht_supported_size becomes static in ggml-metal-common.cpp since nothing outside it needs the size list, which also drops stdint.h from the header again, and ggml_metal_op_mul_mat_use_fwht joins the existing _use_mm declarations under their shared comment instead of carrying its own block. * tests: drop the mismatched-shape Hadamard case I added a case with m != k to cover an abort, but the hint is a promise that src0 is a Hadamard matrix, so src0 is square and dst has the same shape as src1. Every other case in the suite holds to that. The case was not a valid op, and on CPU it compared the FWHT against a real matmul of a non-square src0, which cannot agree. The supports_op and dispatch conditions still come from one predicate, which is what keeps them from disagreeing on contiguity. (cherry picked from commit 9a9f939)
Overview
The CPU FWHT accepts F32 input only. This change adds F16 input.
ggml_compute_forward_fwhtnow takes the source type as a template parameter. The CPU path accepts F16 input and F32 input. The CPU MUL_MAT reference converts an F16src1to F32 when the caller setsGGML_HINT_SRC0_IS_HADAMARD.The change touches
ggml-cpuonly. It is 3 files.Additional information
This is the first change of a small series. The later changes add F16 kernels to the backends, and add the 1024 and 2048 widths. This change is first because it sets the reference behaviour.
There are no test cases here. No backend has an F16 FWHT kernel yet, so a test would have no backend to compare the CPU result against. The test cases come with the first backend change.
test-backend-opsalready has a width-1024 case with the commenttoo big (N>512). A later change covers that width.I did not build the CUDA, Metal, or Vulkan backends. This change does not touch them.
Requirements