Repository navigation
Metal: in-place ops on the owning backend, ROPE_APPLY kernel, group norm batch fix, no FP8/INT8 mul_mat abort - #30
Merged
Merged
Conversation
…Z-Image, Qwen-Image, Wan, LTXV)
…rting at pipeline lookup
…e group, and fixes the reduction buffer reuse
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
Author
|
@codex review |
1 similar comment
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Problem
Four problems in the Metal path of the bundled ggml, found while checking why the macOS prebuilt cannot run on Metal on GitHub's macOS runners and what it costs on Apple Silicon.
ggml_mul_inplace,ggml_add_inplace,ggml_silu_inplace, as in every group-norm affine + SiLU of the UNet and VAE) is a view of that output, but it is still assigned to Metal:GGMLRunner::pin_multi_device_nodespins every node Metal supports, and the scheduler's GPU expansion does the same without a pin. Metal then gets a pointer into a CPU buffer. On the GitHub macOS runner (a VM whose GPU reports no simdgroup support, so GROUP_NORM falls back to the CPU) SD-Turbo on Metal logsggml_metal_buffer_get_id: error: tensor ' (view)' buffer is niland segfaults, on this master and on upstream leejet master alike. A diagnostic build shows the culprit:tensor ' (view)' op=MUL ... view_src='node_169' ... buf_name=CPU. CUDA hides the same assignment on hosts where the GPU can read pageable memory.Rope::apply_rope. On Metalggml_backend_supports_opfails and every q and k of every layer takes the decomposed path (permute, cont, reshape, mul, sub, add, concat).ggml_metal_supports_mul_mat_opreports support for any weight type when the input is not F16, but there are no FP8 or INT8 mul_mat kernels, so the pipeline lookup fails at encode time. sd.cpp only casts FP8 weights to BF16 when the backend says no.Change
Four ggml patches under
scripts/unsloth/ggml-patches/, numbered 0010 to 0013 so they sort after 0007 (#27) and 0008/0009 (#28). They apply in order on 0001 to 0006 alone and on 0001 to 0009:0013-ggml-sched-inplace-op-owner-backend.patch: a pass between the scheduler's pass 4 and pass 5. A non-view op whose result is a view of another tensor is moved to the backend that owns that tensor's memory when its assigned backend cannot use that buffer type and the owner supports the op. Pass 5 then copies its other inputs as usual. Nodes whose backend can already use the memory are untouched, so a graph that runs entirely on one backend schedules exactly as before.0010-ggml-metal-rope-apply.patch:kernel_rope_apply_f32(one threadgroup per token, head and batch row, threads over the rotation pairs, arbitrary source strides, interleaved and split layouts), its pipeline, encoder and support check.0012-ggml-metal-group-norm-threads.patch: one threadgroup per (group, batch) so batches after the first are normalized, the threadgroup grows with the group size up to 1024 threads, and a barrier is added before the second reduction reuses the threadgroup buffer.0011-ggml-metal-no-fp8-int8-mul-mat.patch: Metal reports FP8 (E4M3, E5M2) and INT8 weights as unsupported for MUL_MAT and MUL_MAT_ID, so sd.cpp casts FP8 to BF16 and the scheduler places INT8 matmuls elsewhere instead of aborting.Results
All Metal numbers are from GitHub
macos-15runners (VirtualMac2,1, Apple M1 virtual GPU, 3 vCPU, 7 GB), staging runs on a fork. base = masterafac7a7c, head = this branch, both built exactly like the macOS arm64 release leg (static,-DSD_METAL=ON -DGGML_METAL_EMBED_LIBRARY=ON, deployment target 14.0). Arms are interleaved (order reversed every repeat), one freshsd-cliprocess per measurement, seed 42.The runner GPU reports
MTLGPUFamilyApple5with no simdgroup reduction, simdgroup matrix or bfloat, so ggml-metal sends most ops to the CPU there. A CI-only diagnostic build (not part of this PR) turns those features on with an env var; with ittest-backend-ops -b MTL0passes ADD, MUL, GET_ROWS, CPY, SOFT_MAX, NORM, RMS_NORM, MUL_MAT (1267 cases), FLASH_ATTN_EXT (5004 cases), IM2COL, CONV_2D, UPSCALE, CONCAT, GLU and ROPE with 0 failures, so that mode behaves like a physical M1. Rows marked "forced" use it.In-place op fix (0013), the runner's own GPU (not forced):
buffer is nileach)--backend cpu)The same base crash happens with upstream leejet master on that runner. On CUDA (B200, sd-cli built with
-DSD_CUDA=ON), base and head produce pixel-identical SD-Turbo images (512, 2 steps, also with--clip-on-cpu --vae-on-cpu), 2 runs each. Z-Image-Turbo on CUDA is not run-to-run deterministic here (base vs base 23.6 dB, head vs head 23.2 dB), and base vs head (30.7 dB) is inside that.A host-side repro of the assignment, without Metal: a two-backend graph (CUDA + CPU) where
y = custom(x)can only run on the CPU andz = ggml_mul_inplace(y, w)haswon the GPU. base putszon CUDA0 whileyis on the CPU (auto and pinned like sd.cpp does), head putszon the CPU. The numeric result is correct either way on this host only because its GPU can read pageable memory.ROPE_APPLY kernel (0010), Z-Image-Turbo Q4_K_M, forced:
head sampled faster than base in all 5 interleaved pairs, and
kernel_rope_apply_f32is compiled only in the head runs. The 7 GB runner cannot keep this checkpoint resident (auto-fit streams the DiT weights), so absolute times are not what a 16 GB Mac sees; Qwen-Image-2.1-Turbo Q2_K at 256 px (2 steps, 2 pairs) moved 21.5 s to 20.1 s, inside its spread.Group norm (0012), forced:
GROUP_NORM(ne=[3,5,64,2])(batch 2) fails on base kernels, ERR 0.97 (only batch 0 was normalized), and passes on head.test-backend-ops perf -o GROUP_NORM, 3 interleaved pairs: [64,64,512] 2.06 to 2.19 ms on base vs 0.65 to 0.76 ms on head; [256,256,256] 17.4 to 17.8 ms vs 6.6 to 6.7 ms.FP8 / INT8 (0011): base Metal reports
MUL_MAT(type_a=f8_e4m3, type_b=f32)as supported, although no FP8 or INT8kernel_mul_mv_*/kernel_mul_mm_*exists, so the encoder would hitggml_metal_encoder_set_pipeline: nil Metal pipeline. head reports it unsupported.Accuracy (LPIPS AlexNet, one prompt and seed per cell, same-weights reference, not a bf16 checkpoint): SD-Turbo Metal vs CPU 0.0010 base / 0.0010 head; Z-Image 256 0.0065 / 0.0066; Qwen-Image-2.1 256 0.0057 / 0.0056; SD1.5 cfg 7 0.0301 / 0.0299; Z-Image 512 head vs base Metal 0.0001. None of the four patches is a lossy lever: head sits at base's distance from the CPU reference in every cell.
Metal vs CPU on the same runner, head, forced:
The CPU has only 3 vCPU here, so the ratios overstate what an 8 to 12 core Mac sees. With
--diffusion-fa, SD1.5 sampling on Metal drops from 53.4 s to 16.9 s;--diffusion-conv-directmakes the same UNet trip the GPU watchdog (kIOGPUCommandBufferCallbackErrorTimeout, 3/3) and--vae-conv-directmakes the decode 5.5x slower, so callers should not pass direct conv on Metal.Tests
No test code is part of this PR; the checks below were run locally with
test-backend-opscases kept out of the patches.test-backend-ops -b MTL0, on the runner, forced): ROPE_APPLY cases (interleaved and split layouts, contiguous and permuted sources) 8/8 pass on head and are not supported on base; a batch-2 GROUP_NORM case fails on base and passes on head.zon CUDA0 while its memoryyis in a CPU buffer, in both auto and pinned mode; head runszon the CPU in both modes. The numeric result is identical on both arms, since this GPU can read pageable memory.test-backend-ops test -b CUDA0vs CPU): GROUP_NORM, ROPE, ROPE_APPLY, MUL and ADD pass with no failures on head and base. The Metal kernels cannot run on this host.git apply --checkin order on 0001 to 0006 (this master) and on 0001 to 0009 (with the patches from ggml-cuda: generic DiT and VAE fusions for every family, plus a rope PDL race fix #27 and CPU bundles: runtime-selected ggml CPU variants and repacked quantized weights (1.3x to 3x faster on CPU) #28).Not covered
test-backend-ops).