From 8fcbfc70d9a6ec756de4bdce11f750204a4d5724 Mon Sep 17 00:00:00 2001 From: Joel Teply Date: Tue, 6 Oct 2026 15:07:01 -0500 Subject: [PATCH 1/3] metal: ACC copies the whole of src0 into dst; rows wider than a threadgroup 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 Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo --- ggml/src/ggml-metal/ggml-metal-ops.cpp | 45 ++++++-------------------- tests/test-backend-ops.cpp | 5 +++ 2 files changed, 15 insertions(+), 35 deletions(-) diff --git a/ggml/src/ggml-metal/ggml-metal-ops.cpp b/ggml/src/ggml-metal/ggml-metal-ops.cpp index bfe38daac9ab..d0df91a5f301 100644 --- a/ggml/src/ggml-metal/ggml-metal-ops.cpp +++ b/ggml/src/ggml-metal/ggml-metal-ops.cpp @@ -675,6 +675,8 @@ int ggml_metal_op_repeat(ggml_metal_op_t ctx, int idx) { return 1; } +static int ggml_metal_op_cpy_impl(ggml_metal_op_t ctx, const ggml_tensor * src0, const ggml_tensor * op); + int ggml_metal_op_acc(ggml_metal_op_t ctx, int idx) { ggml_tensor * op = ctx->node(idx); @@ -703,41 +705,14 @@ int ggml_metal_op_acc(ggml_metal_op_t ctx, int idx) { const bool inplace = (bool) ((const int32_t *) op->op_params)[4]; if (!inplace) { - // run a separate kernel to cpy src->dst - // not sure how to avoid this - // TODO: make a simpler cpy_bytes kernel - - //const id pipeline = ctx->pipelines[GGML_METAL_PIPELINE_TYPE_CPY_F32_F32].obj; - auto pipeline = ggml_metal_library_get_pipeline_cpy(lib, op->src[0]->type, op->type); - - ggml_metal_kargs_cpy args = { - /*.nk0 =*/ ne00, - /*.ne00 =*/ ne00, - /*.ne01 =*/ ne01, - /*.ne02 =*/ ne02, - /*.ne03 =*/ ne03, - /*.nb00 =*/ nb00, - /*.nb01 =*/ nb01, - /*.nb02 =*/ nb02, - /*.nb03 =*/ nb03, - /*.ne0 =*/ ne0, - /*.ne1 =*/ ne1, - /*.ne2 =*/ ne2, - /*.ne3 =*/ ne3, - /*.nb0 =*/ nb0, - /*.nb1 =*/ nb1, - /*.nb2 =*/ nb2, - /*.nb3 =*/ nb3, - }; - - ggml_metal_encoder_set_pipeline(enc, pipeline); - ggml_metal_encoder_set_bytes (enc, &args, sizeof(args), 0); - ggml_metal_encoder_set_buffer (enc, ggml_metal_get_buffer_id(op->src[0]), 1); - ggml_metal_encoder_set_buffer (enc, ggml_metal_get_buffer_id(op), 2); - - const int nth = std::min(ggml_metal_pipeline_max_theads_per_threadgroup(pipeline), ne00); - - ggml_metal_encoder_dispatch_threadgroups(enc, ne01, ne02, ne03, nth, 1, 1); + // dst starts as a copy of src0, through the ONE copy path (ggml_metal_op_cpy_impl). A + // hand-rolled copy here dispatched one threadgroup per row with nth = min(max threads, + // ne00), while kernel_cpy_t_t copies one element per thread and covers a wider row + // with more threadgroups (iw0): a row wider than one threadgroup kept stale memory past + // its first nth elements. ACC builds the gradient of a VIEW (Qwen3.5's Q/gate split: + // 4096-wide rows, 3072 of each left stale), so Metal training read garbage there, + // differently per graph layout (2026-10-06). + ggml_metal_op_cpy_impl(ctx, op->src[0], op); ggml_metal_op_concurrency_reset(ctx); } diff --git a/tests/test-backend-ops.cpp b/tests/test-backend-ops.cpp index 894a18fe3b20..917b66c92f67 100644 --- a/tests/test-backend-ops.cpp +++ b/tests/test-backend-ops.cpp @@ -10045,6 +10045,11 @@ static std::vector> make_test_cases_eval() { test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {256, 17, 1, 1}, {256, 16, 1, 1}, -1)); test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {256, 17, 2, 3}, {256, 16, 2, 3}, -1)); test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {256, 17, 2, 3}, {128, 16, 2, 3}, -1)); + // rows wider than one threadgroup: dst = a must be copied whole before b is added (Metal's + // ACC copied only the first threadgroup's worth of each row and left the rest stale, which + // is how a view's gradient (Qwen3.5's Q/gate split, 4096 wide) read garbage in training) + test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {4096, 3, 1, 1}, {1024, 3, 1, 1}, -1)); + test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {4096, 3, 2, 1}, {4096, 2, 2, 1}, -1)); test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {256, 17, 2, 3}, {256, 16, 2, 3}, 1)); test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {256, 17, 2, 3}, {128, 16, 2, 3}, 2)); test_cases.emplace_back(new test_acc(GGML_TYPE_F32, {256, 17, 2, 3}, {64, 16, 2, 3}, 3)); From 5b3e1fa7d51467f82c938b489f1e17203022c83d Mon Sep 17 00:00:00 2001 From: Joel Teply Date: Tue, 6 Oct 2026 15:14:37 -0500 Subject: [PATCH 2/3] metal: SET copies the whole of src0 into dst too (Cormac on #44) 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 Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo --- ggml/src/ggml-metal/ggml-metal-ops.cpp | 39 +++----------------------- tests/test-backend-ops.cpp | 3 ++ 2 files changed, 7 insertions(+), 35 deletions(-) diff --git a/ggml/src/ggml-metal/ggml-metal-ops.cpp b/ggml/src/ggml-metal/ggml-metal-ops.cpp index d0df91a5f301..6dbb19bc19d7 100644 --- a/ggml/src/ggml-metal/ggml-metal-ops.cpp +++ b/ggml/src/ggml-metal/ggml-metal-ops.cpp @@ -2115,41 +2115,10 @@ int ggml_metal_op_set(ggml_metal_op_t ctx, int idx) { const bool inplace = (bool) ((const int32_t *) op->op_params)[4]; if (!inplace) { - // run a separate kernel to cpy src->dst - // not sure how to avoid this - // TODO: make a simpler cpy_bytes kernel - - //const id pipeline = ctx->pipelines[GGML_METAL_PIPELINE_TYPE_CPY_F32_F32].obj; - auto pipeline = ggml_metal_library_get_pipeline_cpy(lib, op->src[0]->type, op->type); - - ggml_metal_kargs_cpy args = { - /*.nk0 =*/ ne00, - /*.ne00 =*/ ne00, - /*.ne01 =*/ ne01, - /*.ne02 =*/ ne02, - /*.ne03 =*/ ne03, - /*.nb00 =*/ nb00, - /*.nb01 =*/ nb01, - /*.nb02 =*/ nb02, - /*.nb03 =*/ nb03, - /*.ne0 =*/ ne0, - /*.ne1 =*/ ne1, - /*.ne2 =*/ ne2, - /*.ne3 =*/ ne3, - /*.nb0 =*/ nb0, - /*.nb1 =*/ nb1, - /*.nb2 =*/ nb2, - /*.nb3 =*/ nb3, - }; - - ggml_metal_encoder_set_pipeline(enc, pipeline); - ggml_metal_encoder_set_bytes (enc, &args, sizeof(args), 0); - ggml_metal_encoder_set_buffer (enc, bid_src0, 1); - ggml_metal_encoder_set_buffer (enc, bid_dst, 2); - - const int nth = std::min(ggml_metal_pipeline_max_theads_per_threadgroup(pipeline), ne00); - - ggml_metal_encoder_dispatch_threadgroups(enc, ne01, ne02, ne03, nth, 1, 1); + // dst starts as a copy of src0, through the ONE copy path (see ggml_metal_op_acc): the + // hand-rolled copy that was here left rows wider than one threadgroup stale past their + // first nth elements, exactly as ACC's did (Cormac on #44) + ggml_metal_op_cpy_impl(ctx, op->src[0], op); ggml_metal_op_concurrency_reset(ctx); } diff --git a/tests/test-backend-ops.cpp b/tests/test-backend-ops.cpp index 917b66c92f67..45ca44168ce9 100644 --- a/tests/test-backend-ops.cpp +++ b/tests/test-backend-ops.cpp @@ -9078,6 +9078,9 @@ static std::vector> make_test_cases_eval() { test_cases.emplace_back(new test_set(GGML_TYPE_F32, GGML_TYPE_F32, {6, 5, 4, 3}, dim, false)); test_cases.emplace_back(new test_set(GGML_TYPE_F32, GGML_TYPE_F32, {6, 5, 4, 3}, dim, true)); } + // a dst row wider than one threadgroup (4096): its copy must be whole before src is set + // into it (Metal's SET, like its ACC, copied only the first threadgroup's worth per row) + test_cases.emplace_back(new test_set(GGML_TYPE_F32, GGML_TYPE_F32, {2048, 2, 1, 1}, 1, false)); for (int dim = 1; dim < GGML_MAX_DIMS; ++dim) { test_cases.emplace_back(new test_set(GGML_TYPE_I32, GGML_TYPE_I32, {6, 5, 4, 3}, dim, false)); From 40bb7d5558f8d2e867c63a62536569865988851a Mon Sep 17 00:00:00 2001 From: Joel Teply Date: Tue, 6 Oct 2026 15:47:19 -0500 Subject: [PATCH 3/3] metal: SET drops the bid_src0 only its old copy used (-Werror on macos-arm64, Cormac on #44) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LoTjvf5j3Ez13g6k8mRkFo --- ggml/src/ggml-metal/ggml-metal-ops.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/ggml/src/ggml-metal/ggml-metal-ops.cpp b/ggml/src/ggml-metal/ggml-metal-ops.cpp index 6dbb19bc19d7..2a1f29e263e2 100644 --- a/ggml/src/ggml-metal/ggml-metal-ops.cpp +++ b/ggml/src/ggml-metal/ggml-metal-ops.cpp @@ -2103,7 +2103,6 @@ int ggml_metal_op_set(ggml_metal_op_t ctx, int idx) { GGML_TENSOR_LOCALS( int32_t, ne, op, ne); GGML_TENSOR_LOCALS(uint64_t, nb, op, nb); - ggml_metal_buffer_id bid_src0 = ggml_metal_get_buffer_id(op->src[0]); ggml_metal_buffer_id bid_src1 = ggml_metal_get_buffer_id(op->src[1]); ggml_metal_buffer_id bid_dst = ggml_metal_get_buffer_id(op);