fix(cpu): stop relayouting a packed weight for kernels this backend does not have - #1126
Merged
Merged
Conversation
…oes not have Closes #1124. The common DefaultCpuOps computed packed matmul wrongly whenever the KernelRegistry was empty — silently, by several times the magnitude of the answer. Ground truth is the weight's own toFloatArray(), so the packed path was disagreeing with its own decoder. matmulWeightTransposed relayouted the weight into the order the *JVM* vectorized kernels read, then handed the result to whatever computed the product. On the JVM that is chooseQuantizedMatmul, which addresses packedData in feed order deliberately and is correct. The common implementation has no such kernel: it decodes through views, and what it was decoding was a tensor whose shape says [in, out] while its blocks still run along the original input dimension. That combination is not canonical and is not input-block-major-at-that-shape; it is a private artifact that only its producer understands, and reading it as either gives plausible garbage. So the common path no longer relayouts. KernelDispatch.matmul already wants the weight output-major, which is exactly the shape an [out, in] weight has, so the canonical view goes in with nothing rearranged. DefaultCpuOpsJvm overrides matmulWeightTransposed to keep the relayout-and-cache path, where the kernels that consume those bytes actually exist and #1096's once-per-weight conversion still pays. Also plumbs block order through to views: PackedBlockStorage.blockOrder, defaulted to ROW_MAJOR so nothing existing changes, forwarded by packedView into Layout.blocked — which has accepted a blockOrder since #1094 and never received one. That closes the information gap #1120 needs. It is worth recording what it cannot do: the relayout artifact above has no honest block order to declare, so marking it was the first fix I tried and it produced zeros. Why no test caught the original bug: skainet-backend-cpu's JVM tests always resolve to DefaultCpuOpsJvm, and its native/JS/Wasm tests do use the common implementation but their platform factories register ScalarKernelProvider first. Every packed-matmul test in the tree, including those added for #973, #1096 and #1108, validated a configuration that was not the broken one. The new test pins the empty-registry configuration explicitly, in commonTest so it runs where that path is real. Gate: scripts/pr-gate.sh — all legs passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
📖 Documentation Preview The documentation has been built successfully for this PR. Generated Files:
Artifacts:
This comment will be updated automatically when the PR is updated. |
This was referenced Aug 25, 2026
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.
Closes #1124.
The common
DefaultCpuOpscomputed packed matmul wrongly whenever theKernelRegistrywas empty — silently, by several times the magnitude of the answer:Ground truth is the weight's own
toFloatArray(), so the packed path was disagreeing with its own decoder.What was actually wrong
matmulWeightTransposedrelayouted the weight into the order the JVM vectorized kernels read, then handed the result to whatever computed the product.On the JVM that is
chooseQuantizedMatmul, which addressespackedDatain feed order deliberately, and is correct. The common implementation has no such kernel — it decodes through views. And what it was decoding is a tensor whose shape says[in, out]while its blocks still run along the original input dimension. That is neither canonical nor input-block-major-at-that-shape: it is a private artifact that only its producer understands, and reading it as either gives plausible garbage.So relayouting for a kernel that does not exist was pure harm.
The fix
The common path decodes the weight where it lies.
KernelDispatch.matmulalready wants the weight output-major, which is exactly the shape an[out, in]weight has, so the canonical view goes straight in with nothing rearranged.DefaultCpuOpsJvmoverridesmatmulWeightTransposedto keep the relayout-and-cache path — there the consuming kernels exist and #1096's once-per-weight conversion still pays. The split now matches reality: fast bytes where something reads them, correct decoding where nothing does.Block order reaches views now
PackedBlockStorage.blockOrder, defaulted toROW_MAJORso nothing existing changes, forwarded bypackedViewintoLayout.blocked— which has accepted ablockOrdersince #1094 and never received one. That closes the information gap #1120 needs.Worth recording what it cannot do, since #1120 will build on it: the relayout artifact above has no honest block order to declare. Marking it
INPUT_BLOCK_MAJORwas the first fix I tried, and it produced zeros — which is what showed the shape and the blocks were inconsistent in the first place.Why nothing caught this
skainet-backend-cpu's JVM tests always resolve toDefaultCpuOpsJvm, whose override is correct.ScalarKernelProviderfirst.So every packed-matmul correctness test in the tree — including the ones added for #973, #1096 and #1108 — validated a configuration that was not the broken one. The gap was structural, not a missed case in any single test.
PackedMatmulEmptyRegistryTestpins the uncovered configuration: four encodings × three shapes, three blocks per row so canonical and feed order are distinguishable (#968), asserted against the weight's own decoder. It lives incommonTest, so it runs on the targets where that path is real.Impact
Production paths register a provider, so applications were not hitting this. What was exposed: anything constructing ops without
PlatformCpuOpsFactory, any target where registration fails — and the fallback's whole purpose, which is to work when nothing else is available.Gate
scripts/pr-gate.sh— all legs passed.🤖 Generated with Claude Code