engine: notice when a tensor's format silently disables the fused Metal decode path - #827
Open
monotophic wants to merge 4 commits into
Open
engine: notice when a tensor's format silently disables the fused Metal decode path#827monotophic wants to merge 4 commits into
monotophic wants to merge 4 commits into
Conversation
…ntly falls back) Both fused Metal decode-attention gates (attention_rows and layer_forward_rows) hard-require l->kv_b.fmt==2. A v1-class mixed-precision container can mint kv_b_proj at a different format, which today silently pushes every layer's decode attention onto the CPU absorb path with no signal anywhere -- measured on this box as 33.4s -> 16.4-22.1s attention wall time per 255 decode tokens, +22% end-to-end once cured. Add kvb_fmt_gate_notice(), called once from model_init() after all layer kv_b tensors are resolved: if Metal is enabled and any layer's kv_b.fmt!=2, print one [METAL]-tagged stderr line naming the actual fmt, the fmt=2 requirement, the CPU-fallback consequence, affected-layer count, and the --kvb-bits 4 remedy. Notice only -- no gate or behavior change. Scope is the main layers (m->L): the MTP head is deliberately excluded -- common containers ship it at INT8 by design, and the fused layer-decode gate already excludes it (li<n_layers). g_metal_enabled moves from an #ifdef COLI_METAL-only declaration to an unconditional one (still 0-init, still only ever written under COLI_METAL) so the notice and its test compile identically on every platform without a Metal framework link. Test: tests/test_kvb_notice.c, wired into test-c, exercises the notice function directly against a hand-built Model (no snapshot, no Metal link) via the test_int3_load.c loader-seam pattern. Portable stderr capture via freopen + dup/dup2 -- no child-process spawn/reap primitives.
…d to match The kv_b notice (previous commit) duplicated the fused gates' format condition, and the gates have since grown: both fused Metal decode entries now also require q_a/q_b/kv_a/o -- and layer_forward_rows additionally sh_gate/sh_up/sh_down -- to sit on the metal_fused_fmt_ok() allowlist (fmt 1/2/3/4). A container minting any of those tensors off-allowlist silently loses the fused path per layer, and the kv_b-only notice says nothing: the same silent-fallback trap, one tensor over. Extract the per-layer condition into metal_fused_layer_fmt_miss(): a pure bitmask (one METAL_FUSED_* bit per bound tensor -- kv_b pinned to fmt==2, the rest on the allowlist; sh_* checked on sparse layers only, since dense layers never load them and never reach the fused layer CB). Both gates now test it against the mask of tensors their kernel binds (METAL_FUSED_ATTN_TENSORS / METAL_FUSED_LAYER_TENSORS) -- decision logic identical in effect, purely reorganized -- and the notice (renamed metal_fmt_gate_notice) reports every set bit, so notice and gates consume one condition and cannot drift apart again. Notice output stays bounded: one [METAL] line per offending tensor KIND (max 8 incl. kv_b), never per layer. kv_b keeps its exact line incl. the --kvb-bits 4 remedy; allowlist kinds get kind name, actual fmt, affected layer count, the fmt 1/2/3/4 requirement, and the CPU-fallback consequence. Still silent when clean or when Metal is off; the MTP head (m->mtpL, INT8 by design, never gated) stays excluded. test_fp8_load.c Part F's grep-pin counted the 11 scattered metal_fused_fmt_ok(l->...) call sites this commit consolidates; the pin is re-expressed for the new shape with the same intent -- exactly 7 call sites, all inside the helper, and each gate consulting the helper against its own tensor mask. Test: test_kvb_notice.c extended -- per-kind misses for all seven allowlist tensors, multi-kind bounded output (8 lines on a 40-layer all-bad model, not 320), all-pass silence across all four admitted formats, Metal-off silence, MTP exclusion, dense-layer sh_* exemption (a real GLM container has dense layers; without the sparse guard every load would false-positive on sh_*), and a truth table pinning the shared predicate's bit semantics.
…mposition Review round on the widened notice; gates and predicate logic untouched. The sh_gate/sh_up/sh_down notice lines printed affected/n_layers even though their population is sparse layers only -- on a 61-layer GLM with one dense layer they could never truthfully say 61/61. Their denominator is now the sparse-layer count and those lines say "sparse layer(s)"; every other kind keeps n_layers. One fprintf changed. The notice's format-only semantics are now stated in its block comment: a line names a FORMAT obstacle and does not claim the fused path would otherwise engage -- the gates' further preconditions (GLM dims, batch shape, expert config) are properties of the runtime call, deliberately not duplicated here, since re-stating them would recreate exactly the duplicated-condition drift the shared predicate exists to kill. test_kvb_notice.c: the truth table pinned the helper's bits but not which bits each GATE mask contains -- a mask silently dropping a member (e.g. METAL_FUSED_O falling out of METAL_FUSED_ATTN_TENSORS) failed open on the fused path with the whole suite still green. New part (j) asserts, per kind, the masked gate booleans the gates actually compute against membership stated independently of the enum definitions (attn mask: kv_b+q_a+q_b+kv_a+o, not sh_*; layer mask: all 8). New part (i) covers the mixed dense/sparse shape (61 layers, layer 0 dense, everything bad): exactly 8 bounded lines, 61/61 on attn-bound kinds, 60/60 sparse on sh_*. Existing count pins updated for the new denominators.
The registry documents what each format IS; the fused Metal decode path is the first engine consumer that places format REQUIREMENTS on specific tensors (kv_b_proj pinned to fmt=2, the other fused-bound tensors on the fmt 1/2/3/4 allowlist), with a load-time [METAL] stderr notice when a container misses them. Add a short consumer-side note under "Known formats" anchoring the shared predicate, both gate sites, and the notice.
Contributor
|
Second Apple datapoint for this, from the other side of the same wall. I hit this exact path last week converting a GLM-5.2 REAP-504B container to fmt=6 on an M5 Curing it (rebuilding the container per-row so dense matches, The load-time notice would have saved me a full reconversion of diagnosis. Worth having. |
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
2 participants
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.
Authored by Fable 5 in Claude Code, analysis in partnership with @monotophic.
A container can mint any fused-bound tensor at a format the fused Metal decode
kernels don't accept — and today that silently pushes the affected layers'
decode attention onto the CPU path with no signal anywhere. We measured the
kv_b_proj case on an M5 Max at +22% end-to-end once cured. This PR makes the
condition visible at load, and makes it impossible for the diagnostic to drift
out of sync with the gates it describes:
metal_fused_layer_fmt_miss, a purebitmask over the 8 fused-bound tensor kinds) now backs BOTH fused-gate call
sites (
attention_rows,layer_forward_rows) and a new one-line-per-kind[METAL]stderr notice atmodel_init. Gate behavior is bit-identical —see the matrix below. The notice names the tensor kind, the offending fmt,
the affected-layer count, the fmt 1/2/3/4 allowlist requirement (kv_b:
fmt=2 exactly, with the
--kvb-bits 4remedy), and stays silent on cleancontainers and non-Metal builds. sh_* kinds count over the sparse-layer
population (the only layers that load them); the MTP head is excluded by
design (commonly INT8, never fused).
obstacle; it does not claim the fused path would otherwise engage (the
gates' dims/batch/config preconditions are deliberately not duplicated).
requirements with anchors to the predicate, both gates, and the notice.
test_kvb_notice.c), incl. per-bit gate-mask membership; 7-mutation battery — every mutation caught, incl. a single mask bit droppedmake checkgreen at every commit; METAL=1 build zero warnings vs baselineCommit structure, verification detail, and observations
Commits by origin:
2078199— the original kv_b-only notice, mechanically rebased ontocurrent dev. Only resolution of note: the hand-maintained
TEST_BINSaddition was dropped — dev now auto-derives test binaries from build rules,
which removes that whole conflict class.
988223d— the widening (all 8 fused-bound kinds) + the shared-predicateconsolidation (replaces 11 scattered allowlist call sites with 7 inside one
helper; the gates' comment blocks updated to match).
9fe7894— review round: sh_* denominators (sparse-layer population),per-bit mask-membership test (a dropped mask bit previously passed the
whole suite), mixed dense/sparse test shape, format-only semantics
documented in the notice's comment.
353f5ea— the FORMATS.md consumer-requirements note.Verification: the equivalence sweep enumerated fmt ∈ {0..8} per tensor ×
sparse ∈ {0,1} (all values outside {1,2,3,4} are one behavioral class, so the
domain covers every class with margin) and compared the old gate expressions,
transcribed verbatim from the base revision, against the shipped predicate at
-O3 and -O0. Compiler output at -O3 confirms the helper inlines into both
gates, the attention gate's masked-away sh_* reads are dead-code-eliminated,
and the remaining check vectorizes — it is not slower than the chain it
replaces. Dense layers never load sh_* tensors, so those kinds are checked on
sparse layers only (a literal all-layers check would false-positive on every
real GLM load). Test suite: kv_b miss · each allowlist kind · multi-kind
bounded output (≤8 lines) · all-pass silence · Metal-off silence · MTP
exclusion · dense-exemption · predicate truth table · mixed dense/sparse
counts · per-bit mask membership.
Observations for maintainers (nothing here is changed by this PR):
make metal-testdoes not compile on current dev in eitherCOLI_METAL_RESSETstate:quant.h:483fp8_nblk(returnsint64_t)collides with
tests/test_backend_metal.mm:99's localstatic int fp8_nblk. Pre-exists this branch (verified at the untouched base). Happyto send the trivial fix separately.
metal_fused_fmt_okcomment block said "fmt 1/2/4" while the predicateincludes
fmt==3; the consolidated comment now just names the allowlist.attention_rowsgained the(S<=4 || g_metal_prefill)prefill scope whilelayer_forward_rowsdid not — the two gates have diverged on batch scope(flagging only; format conditions are what this PR unifies).
the notice's kind arrays must move together — the per-bit membership test
now enforces the mask half of that lockstep.