ggml-metal: fix copy bounds and F32 precision, optimize short-K matmul - #609
Open
davidxifeng wants to merge 3 commits into
Open
davidxifeng wants to merge 3 commits into
davidxifeng wants to merge 3 commits into
Conversation
davidxifeng
force-pushed
the
codex/metal-f32-copy-small-k
branch
from
September 19, 2026 09:34
a17917b to
c3ad8d1
Compare
Owner
|
@davidxifeng Run the test external/ggml/tests/test-metal-f32-matmul.cpp
It's unclear what other models could hit the slow region and regress, and how many models would benefit from the correctness fixes. Could you provide more information? I need to understand the trade-offs before deciding on the next steps. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows up on the ggml changes removed from #599, now that ZipVoice has merged. It separates two correctness fixes from the short-K performance optimization into three commits, with no model-specific changes.
kernel_cpy_t_tso short, permuted rows cannot overwrite subsequent channels or memory past the logical output tensor. A regression checks exact output and tail sentinels across 24 shapes, five repetitions each.GGML_PREC_F32, with matching shared-memory sizing and offsets. The existing half staging loses small differences and can turn finite inputs around 70000 into nonfinite outputs. Default precision and other input types retain their existing kernels.Validation on Apple M3 against upstream
a7b58a6:The technical report is included below.
Technical report and reproduction instructions
Metal copy bounds, explicit F32 precision, and short-K matmul
Scope
These changes follow up on the ggml changes removed from ZipVoice PR #599.
They contain two correctness fixes and one performance optimization, in separate
commits. No model implementation, package specification, CUDA kernel, or default
precision policy is changed.
The branch is based on upstream
a7b58a6and preserves the subsequently mergedMetal BF16 rounding work. Validation was performed on an Apple M3 on 2026-09-19.
Strided copy bounds
kernel_cpy_t_tbatches short rows into a threadgroup. The last group can includerows with
i01 >= ne01. Without a guard, those threads calculate source addressesusing another channel's strides and write past the logical destination tensor.
The added early return excludes these padded rows.
The regression uses a real permutation from
[C,K,R,B]to[K,R,C,B], followedby
ggml_cpy. Both backing allocations contain explicit extra space, and theoutput tail is initialized with sentinels. Thus the test detects logical
out-of-bounds writes without relying on an allocation fault. It checks 24 shapes,
five repetitions each, including singleton rows and widths around the batching
threshold. Any incorrect output or changed sentinel fails the executable.
In the original local audit, K=32, R=1, C=5, B=2 produced up to 272 incorrect output
values and overwrote 224 float sentinels. All patched cases were correct.
Explicit F32 matrix multiplication
The existing F32-by-F32 tiled kernel stages operands as
half. Float accumulationcannot recover information lost during that conversion, even when the operation
requests
GGML_PREC_F32. The new specialization stages both operands as float;pipeline selection, shared-memory allocation, and the B-buffer offset are updated
together. Default precision and other input types retain their existing kernels.
The regression compares GPU results with double accumulation over the actual
input floats. It covers ordinary values, values around 70000 (outside the finite
FP16 range), cancellation of small differences, partial tiles, and batch
broadcasting. Explicit F32 cases must have finite outputs and maximum error
normalized by
1 + sum(abs(products))below2e-6. Default-precision and F16cases are also executed as controls. Missing Metal hardware returns CTest skip
code 77 rather than reporting a pass.
Original audit, K=128, M=67, N=35, batch=2:
Short-K dispatch
Previously, tiled MM required K >= 64. The added path allows 32 <= K < 64 only
for F32-by-F32 operands with explicit F32 precision, M >= 64 and N >= 32, while
retaining the existing layout and device eligibility checks. The new precision
kernel is a prerequisite: routing these contractions through the old half-staged
kernel would introduce an avoidable precision regression.
Tests include K=31/32/33/48/63/64/65/128 and M/N values immediately below and above
the dispatch thresholds. Timing is printed but is not a pass/fail criterion.
Changing the reduction order means the results are not necessarily bit-identical.
Model-level evidence from the preceding isolated audit
The following measurements predate the rebase onto
a7b58a6; they are retainedas model-level evidence, not presented as a new full-model benchmark of this PR.
The same F32 ZipVoice weights and model code were linked against original ggml,
correctness fixes only, and the full patch. CPU served as the reference.
For a 2048-frame decoder velocity evaluation, each process warmed up once and
averaged five evaluations; the table gives the median of three process means:
The short-K change reduced this workload's time by about 44% (1.79x speedup).
Full patch versus correctness-only output relative RMSE was 8.85730e-7. These are
single decoder evaluations, not end-to-end audio RTF measurements.
Q8 CPU/Metal differences remain: the F32 specialization does not unify quantized
backend arithmetic. For example, Q8 decoder relative RMSE at 128 frames changed
only from 0.00991154 to 0.00989252. No listening-quality claim is made.
Reproduction on this branch
The top-level audio.cpp build disables upstream ggml tests, so configure ggml
standalone:
cmake -S external/ggml -B build/ggml-metal-review \ -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF \ -DGGML_METAL=ON -DGGML_BUILD_TESTS=ON -DGGML_BUILD_EXAMPLES=OFF cmake --build build/ggml-metal-review \ --target test-metal-copy-bounds test-metal-f32-matmul -j 8 ctest --test-dir build/ggml-metal-review \ -R 'test-metal-(copy-bounds|f32-matmul)' --output-on-failureBoth tests pass on M3 with the rebased changes. Both also fail on the isolated
unmodified
a7b58a6baseline: copy output/sentinel corruption and excessiveexplicit-F32 matmul error, respectively. To repeat this negative control, copy the two test sources and their CMake registrations into an isolated
checkout of
a7b58a6, build with the same options, and run the same command.Limits and review considerations
Only Apple M3 was exercised. Other Apple GPUs, especially the M5 tensor API path,
require additional validation. This is not a claim of regression coverage for all
models or backends. Explicit F32 can cost more than half-staged computation on
some shapes; the short-K speedup should not be generalized to every matrix size.
The copy fix affects other users of the generic strided-copy kernel, which is why
it includes direct output and sentinel regression checks.