Repository navigation
metal: add F16 input to the FWHT - #29094
Conversation
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.
ggerganov
left a comment
There was a problem hiding this comment.
Not sure how performance critical this kernel is, but you might want to check the suggestions below.
| reg[i] = float(src[i*NW + lane])*scale; | ||
| } | ||
| for (int i = 1; i < NW; i *= 2) { | ||
| for (int j = 0; j < NE; j++) { |
There was a problem hiding this comment.
Have you tried explicitly unrolling some of these loops using FOR_UNROLL? My experience is that it can help in many cases.
There was a problem hiding this comment.
Have you tried if this improves perf:
| reg[j] = val2 - val + 2*((lane & i) == 0)*val; |
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.
|
Pushed a fix on top, so this needs another look. The supports_op branch I added admitted an F16 src1 based on the type, the hint and the width, but the dispatch also requires src1 and dst to be contiguous and the same shape. An op that passed the first check and failed the second fell through to the generic path, which has no F32 src0 by F16 src1 kernel, and aborted on a nil pipeline. A Hadamard-hinted matmul with an F16 src1 and m != k reproduces it. The condition now lives in one place, ggml_metal_use_fwht, and both supports_op and the dispatch call it, so they cannot disagree again. I added the case that aborted as a test. |
|
@ggerganov Thanks trying the suggestions now to see if it helps. (it would need this PR and 1-2 of other draft PR's @bri-prism has sent). https://huggingface.co/prism-ml/Ternary-Bonsai-2-27B-gguf-dev |
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.
| // supported FWHT sizes, must stay in sync with the | ||
| // kernel_fwht_<type>_<N> templates in misc.metal | ||
| static inline bool ggml_metal_fwht_supported_size(int64_t n) { | ||
| return n == 64 || n == 128 || n == 256 || n == 512; | ||
| } | ||
|
|
||
| // whether a Hadamard-hinted MUL_MAT is handled by the FWHT kernels. supports_op and the | ||
| // dispatch must ask the same question: an F16 src1 that is admitted but then falls through | ||
| // reaches the generic path, which has no F32 src0 by F16 src1 kernel. | ||
| static inline bool ggml_metal_use_fwht(const struct ggml_tensor * op) { | ||
| // op_params[1] is the mul_mat hint; read directly so this header needs only ggml.h | ||
| return op->op_params[1] == GGML_HINT_SRC0_IS_HADAMARD && | ||
| op->type == GGML_TYPE_F32 && | ||
| (op->src[1]->type == GGML_TYPE_F32 || op->src[1]->type == GGML_TYPE_F16) && | ||
| ggml_is_contiguous(op->src[1]) && | ||
| ggml_is_contiguous(op) && | ||
| ggml_are_same_shape(op->src[1], op) && | ||
| ggml_metal_fwht_supported_size(op->src[1]->ne[0]); | ||
| } |
There was a problem hiding this comment.
These should not be implemented here. See the ggml_metal_op_mul_mat_use_mm pattern.
122ed55 to
09306f6
Compare
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.
09306f6 to
b939854
Compare
|
I can't push to your branch - please apply this patch: diff --git a/ggml/src/ggml-metal/ggml-metal-common.cpp b/ggml/src/ggml-metal/ggml-metal-common.cpp
index 58a0bb038e..5ce5352da2 100644
--- a/ggml/src/ggml-metal/ggml-metal-common.cpp
+++ b/ggml/src/ggml-metal/ggml-metal-common.cpp
@@ -7,17 +7,8 @@
#include <vector>
-bool ggml_metal_op_mul_mat_use_mm(const struct ggml_tensor * op, bool has_simdgroup_mm) {
- const int64_t ne00 = op->src[0]->ne[0];
- const int64_t ne11 = op->src[1]->ne[1];
-
- return !ggml_is_transposed(op->src[0]) &&
- !ggml_is_transposed(op->src[1]) &&
- has_simdgroup_mm && ne00 >= 64 && ne11 > 8;
-}
-
// must stay in sync with the kernel_fwht_<type>_<N> templates in misc.metal
-bool ggml_metal_fwht_supported_size(int64_t n) {
+static bool ggml_metal_fwht_supported_size(int64_t n) {
return n == 64 || n == 128 || n == 256 || n == 512;
}
@@ -34,6 +25,15 @@ bool ggml_metal_op_mul_mat_use_fwht(const struct ggml_tensor * op) {
ggml_metal_fwht_supported_size(op->src[1]->ne[0]);
}
+bool ggml_metal_op_mul_mat_use_mm(const struct ggml_tensor * op, bool has_simdgroup_mm) {
+ const int64_t ne00 = op->src[0]->ne[0];
+ const int64_t ne11 = op->src[1]->ne[1];
+
+ return !ggml_is_transposed(op->src[0]) &&
+ !ggml_is_transposed(op->src[1]) &&
+ has_simdgroup_mm && ne00 >= 64 && ne11 > 8;
+}
+
bool ggml_metal_op_mul_mat_id_use_mm(const struct ggml_tensor * op, bool has_simdgroup_mm) {
const int64_t ne00 = op->src[0]->ne[0];
const int64_t ne21 = op->src[2]->ne[1];
diff --git a/ggml/src/ggml-metal/ggml-metal-common.h b/ggml/src/ggml-metal/ggml-metal-common.h
index a9986bf245..e6a28d032d 100644
--- a/ggml/src/ggml-metal/ggml-metal-common.h
+++ b/ggml/src/ggml-metal/ggml-metal-common.h
@@ -3,7 +3,6 @@
#pragma once
#include <stdbool.h>
-#include <stdint.h>
#ifdef __cplusplus
extern "C" {
@@ -49,13 +48,10 @@ bool ggml_mem_ranges_check(ggml_mem_ranges_t mrs, const struct ggml_tensor * ten
void ggml_graph_optimize(struct ggml_cgraph * gf);
// mat-mat vs mat-vec dispatch; used by both supports_op and ggml_metal_op_mul_mat*
+bool ggml_metal_op_mul_mat_use_fwht (const struct ggml_tensor * op);
bool ggml_metal_op_mul_mat_use_mm (const struct ggml_tensor * op, bool has_simdgroup_mm);
bool ggml_metal_op_mul_mat_id_use_mm(const struct ggml_tensor * op, bool has_simdgroup_mm);
-// FWHT dispatch; used by both supports_op and ggml_metal_op_mul_mat
-bool ggml_metal_fwht_supported_size (int64_t n);
-bool ggml_metal_op_mul_mat_use_fwht (const struct ggml_tensor * op);
-
#ifdef __cplusplus
}
#endif |
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.
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.
|
Pushed fixes for all three review comments. The branchless butterfly select and the dispatch predicate move are both applied as suggested (measured +3.0% on M5 Pro for the butterfly change). I also tried FOR_UNROLL on the same loops but it made no consistent difference across rounds, so I left that one out. Ready for another look whenever you have time. |
* 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.
* 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 Metal FWHT kernel accepts F32 input only. This 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_opaccepts an F16src1for the Hadamard hint at the four sizes the kernels cover. Every other F16
src1pathstill goes through
ggml_metal_supports_mul_mat_op.This is the backend follow-up to #27779, which added F16 input to the CPU FWHT and noted
that the test cases would come with the first backend kernel. Those cases are included here.
Additional information
ggml_metal_fwht_supported_sizemoves fromggml-metal-ops.cpptoggml-metal-device.hso
supports_opinggml-metal-device.mcan use the same list. It does not go inggml-metal-impl.h, which the shaders include throughcommon.h.The F16 gate in
supports_opis restricted to the sizes that have kernels. Outside thosesizes an F16
src1keeps the existing behaviour, so the fallback never sees an F32 src0against an F16 src1.
Tested on an M5 Pro with
test-backend-ops:MUL_MAT_HADAMARD16/16, including the seven new F16 casesMUL_MAT1265/1265, no regressionsRequirements
structure of the CPU version in ggml-cpu: add F16 input to the FWHT #27779, and to format this description to the PR template.
I reviewed every line and take full responsibility for the changes.