Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joelteply merged 3 commits into from
Oct 7, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8fcbfc7
metal: ACC copies the whole of src0 into dst; rows wider than a threa…
joelteply Oct 6, 2026
5b3e1fa
metal: SET copies the whole of src0 into dst too (Cormac on #44)
joelteply Oct 6, 2026
40bb7d5
metal: SET drops the bid_src0 only its old copy used (-Werror on maco…
joelteply Oct 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 14 additions & 71 deletions ggml/src/ggml-metal/ggml-metal-ops.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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<MTLComputePipelineState> 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);
}
Expand Down Expand Up @@ -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);

Expand All @@ -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<MTLComputePipelineState> 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);
}
Expand Down
8 changes: 8 additions & 0 deletions tests/test-backend-ops.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9078,6 +9078,9 @@ static std::vector<std::unique_ptr<test_case>> 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));
Expand Down Expand Up @@ -10045,6 +10048,11 @@ static std::vector<std::unique_ptr<test_case>> 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));
Expand Down
Loading