Repository navigation
Fix qvm_split_k incorrect batch stride calculation - #3497
Merged
Merged
Conversation
magicnight
added a commit
to magicnight/mlx
that referenced
this pull request
Aug 8, 2026
sinistersoundworks
pushed a commit
to sinistersoundworks/Mac-MLX
that referenced
this pull request
Aug 25, 2026
…urces so they apply The release scan added in magicnight#108 listed 33 upstream fixes the fork does not carry. Triaging the three that touch quantized matmul — the path most users actually run — found two that reach us and one that cannot. ml-explore/mlx#3497: qvm mishandles a stride-0 batch dimension. That is exactly the shape mlx-swift-lm's quantizedScaledDotProductAttention produces, where a quantized KV cache is broadcast across GQA repeat groups with expandDimensions. Only the non-transposed product (scores @ values) reaches the affected kernel, and only for M >= 2, which is why ordinary single-token decode never showed it while speculative decoding sits squarely in range. Not hardware-gated — upstream reproduced on an M4 Pro. Measured here before the fix: max |diff| 0.139 at N=2048 and 0.104 at N=4096 against a dequantized reference, versus ~2e-4 quantization noise, with M=1 correct — matching upstream's numbers and its stated boundary. ml-explore/mlx#3631: NAX qmm edge-tile bounds computed in short(), wrapping past 32767, corrupting a column band for unaligned N above 2^15. Reached through an lm_head whose vocab is unaligned. MiniCPM3-4B's 73448 is one, and one this project lists as supported; every other vocab we support is a multiple of 64 and takes the aligned branch. Measured here before the fix: max |diff| 236 at N=73448, K=2560, M=16. ml-explore/mlx#3854 is NOT carried: upstream states affine quantization is unaffected, and every model this project loads is affine. Bumping the submodule was not enough. mlx-swift does not compile the submodule's Metal kernel headers — it embeds them, stringified, into checked-in files under Source/Cmlx/mlx-generated, and the JIT compiles those. After the bump both tests still failed with byte-identical numbers, because the generated copies still held the pre-fix kernel text. The fork now regenerates them. #3810 hid this trap earlier: it changed host C++, which is compiled from source, so a plain submodule bump was sufficient there and looked like the rule. QuantizedMatmulParityTests pins both, using upstream's own repro shapes, and includes the M=1 case as a control so the M>=2 boundary is asserted rather than assumed.
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.
Fixes #3480 .