Skip to content

metal: ACC copies the whole of src0 into dst; rows wider than a threadgroup kept stale memory - #44

Merged
joelteply merged 3 commits into
feat/props-weight-residencyfrom
fix/metal-acc-copies-every-row-element
Oct 7, 2026
Merged

joelteply merged 3 commits into
feat/props-weight-residencyfrom
fix/metal-acc-copies-every-row-element

Conversation

@joelteply

Copy link
Copy Markdown

ggml_metal_op_acc made dst a copy of src0 with its own dispatch: one threadgroup per row, nth =
min(max threads, ne00). kernel_cpy_t_t copies ONE element per thread and covers a row wider
than a threadgroup with more threadgroups (iw0 = tgpig.x / ne01), which this dispatch never
launched. So every Metal ACC whose src0 rows were wider than one threadgroup left dst stale past
the first nth elements of each row.

ACC builds the gradient of a VIEW: ggml's backward accumulates a view's gradient into a zeroed
copy of its source. Qwen3.5's attention splits Q and the gate out of one 4096-wide projection,
so the gradient of that projection carried 3072 stale floats per row on Metal, a different 3072
per graph layout. Measured on Qwen3.5 0.8B (M5), the first differing gradient between recompute
OFF and ON was exactly this ACC: identical inputs (byte-hashed in both runs), different outputs,
and 3.66M non-zero elements where only the 2.1M added ones should be non-zero, first difference
at index 1024.

The copy now goes through ggml_metal_op_cpy_impl, the one copy path, which dispatches the
iw0 threadgroups.

Tests: test_acc with 4096-wide rows (two shapes). Known-positive: with the old copy they FAIL
(ERR 1.23 and 0.30, 6/8); with this, 8/8 pass on MTL0.

End to end, on the 0.8B (window 1024, seed 7), with llama.cpp #43:

🤖 Generated with Claude Code

https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo

…dgroup kept stale memory

ggml_metal_op_acc made dst a copy of src0 with its own dispatch: one threadgroup per row, nth =
min(max threads, ne00). kernel_cpy_t_t copies ONE element per thread and covers a row wider
than a threadgroup with more threadgroups (iw0 = tgpig.x / ne01), which this dispatch never
launched. So every Metal ACC whose src0 rows were wider than one threadgroup left dst stale past
the first nth elements of each row.

ACC builds the gradient of a VIEW: ggml's backward accumulates a view's gradient into a zeroed
copy of its source. Qwen3.5's attention splits Q and the gate out of one 4096-wide projection,
so the gradient of that projection carried 3072 stale floats per row on Metal, a different 3072
per graph layout. Measured on Qwen3.5 0.8B (M5), the first differing gradient between recompute
OFF and ON was exactly this ACC: identical inputs (byte-hashed in both runs), different outputs,
and 3.66M non-zero elements where only the 2.1M added ones should be non-zero, first difference
at index 1024.

The copy now goes through ggml_metal_op_cpy_impl, the one copy path, which dispatches the
iw0 threadgroups.

Tests: test_acc with 4096-wide rows (two shapes). Known-positive: with the old copy they FAIL
(ERR 1.23 and 0.30, 6/8); with this, 8/8 pass on MTL0.

End to end, on the 0.8B (window 1024, seed 7), with llama.cpp #43:
- recompute OFF, full recompute, layer-3-only: train 1.8693231 / eval 1.9235938, bit-identical
  (they differed by 6e-5 to 5e-4 before, and layer-3-only gave NaN without #43)
- Metal against CPU: eval 1.9235938 vs 1.9236773; before, 1.9291925 vs 1.9236773. The 0.0055
  gap read as the CPU's activation quantization was this stale memory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo
@joelteply

Copy link
Copy Markdown
Author

One addition at 8fcbfc7, then approve: ggml_metal_op_set has the SAME bug.

Its non-inplace branch (the same "run a separate kernel to cpy src->dst / not sure how to avoid this" comment, at line 2143 here) builds kargs_cpy and dispatches ne01 x ne02 x ne03 threadgroups with nth = min(max_threads, ne00). That is the dispatch #44 just removed from ACC. So a SET whose rows are wider than one threadgroup keeps the same stale tail in dst. Route it through ggml_metal_op_cpy_impl too, and add a wide-row test_set beside the new test_acc cases. Then there is one copy path, and no third copy of this dispatch can drift again.

The ACC fix itself is right. The differential that found it (hash every grad for node, OFF vs ON, first divergence with byte-identical inputs) is the right instrument. Reusing ggml_metal_op_cpy_impl is the one-copy-path answer, and the 4096-wide test_acc cases fail before and pass after.

Upstream: both hand-rolled copies (ACC and SET) are in ggml-org/llama.cpp's ggml-metal-ops.cpp today, with the same comment. This is a real upstream bug, and every Metal training user with a wide view's gradient hits it. Worth a small upstream PR with the two test_acc/test_set cases. They fail on stock Metal, which makes the case on its own.

ggml_metal_op_set had the same hand-rolled copy as ACC (one threadgroup per row, nth =
min(max threads, ne00), without the iw0 threadgroups kernel_cpy_t_t needs for a wide row), so a
non-inplace SET into rows wider than a threadgroup left the same stale tail. It now goes through
ggml_metal_op_cpy_impl as well.

Test: test_set with a 4096-wide dst. Known-positive: with the old copy it FAILS (ERR 0.72,
12/13); with this, 13/13 SET and 8/8 ACC pass on MTL0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo
@joelteply

Copy link
Copy Markdown
Author

5b3e1fa: SET routed through ggml_metal_op_cpy_impl as well, with a 4096-wide test_set. Known-positive: the old SET copy FAILS it (ERR 0.72, 12/13); the fix passes 13/13 SET and 8/8 ACC. Agreed on upstream: both copies are in ggml-org's tree, and the two wide tests fail there as they did here. I'll open it once this lands.

…s-arm64, Cormac on #44)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo
@joelteply

Copy link
Copy Markdown
Author

APPROVED at 40bb7d5.

The delta from 5b3e1fa is the one unused bid_src0 declaration, and macos-latest-arm64 builds again. ACC and SET both copy src0 through ggml_metal_op_cpy_impl (the one copy path), with wide-row test_acc and test_set cases that fail on the old dispatch and pass on this one. With #43 this closes 9355b90c: recompute OFF = full = layer-3-only, bit-identical on Metal, and within 8e-5 of CPU. No failed checks; the self-hosted GPU runners are pending as on #43. Worth taking upstream with the two tests, since the same hand-rolled copies are in ggml-org today.

@joelteply
joelteply merged commit 1b7b5a7 into feat/props-weight-residency Oct 7, 2026
18 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant