diff --git a/ggml/src/ggml-metal/ggml-metal-ops.cpp b/ggml/src/ggml-metal/ggml-metal-ops.cpp index bfe38daac9ab..2a1f29e263e2 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); } @@ -2128,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); @@ -2140,41 +2114,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 894a18fe3b20..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)); @@ -10045,6 +10048,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));