Re-disable qmm_n_nax and fix group_size < 64 - #4202
Conversation
The group_size=32 QuantizedBlockLoader specialization advanced the scale (and bias) pointer by n_groups * group_stride when stepping between reduction tiles. For the non-transposed kernel the scale plane is [K, N / group_size], so a 64-row reduction tile is group_stride = 64 * N / group_size scale entries, not n_groups times that. The doubled step read out of bounds from the second K-tile on, producing garbage (and NaN under some inputs) for group_size=32 quantized_matmul(transpose=False) on Metal whenever K > 64. Also fixes the same pattern in the fp (fp8/MX) loader. Adds a regression over the gs=32 non-transposed path covering a single and multiple 64-row K-tiles. Fixes ml-explore#4201.
322b92f to
500cda8
Compare
zcbenz
left a comment
There was a problem hiding this comment.
Thanks for the fix, I should have ran the full test suite when testing the PR 🤦
So apparently the whole qmm_n is still WIP and I'm re-disabling it until a full review and benchmarking.
414a858 to
ca42d4c
Compare
|
Flagging a footgun this left behind, since #4051 was mine. After this PR the guard is: bool has_nax_kernel =
metal::is_nax_available() && (transpose || mode == "affine");
if (has_nax_kernel && transpose && (K % 64 == 0) && ...
It needs one. Folding the precondition into the variable makes the name true and keeps it from re-arming, without changing behaviour: bool has_nax_kernel = metal::is_nax_available() &&
(transpose || (mode == "affine" && N % 64 == 0));
if (has_nax_kernel && transpose && (K % 64 == 0) && ...Unrelated but adjacent: |
Fixes #4201. Root-cause fix (supersedes the earlier workaround in this PR).
Root cause
The
group_size=32QuantizedBlockLoaderspecialization(
quantized_nax.h) advanced the scale/bias pointer byn_groups * group_stridewhen stepping between reduction tiles. For thenon-transposed NAX kernel (#4051) the scale plane is
[K, N / group_size],so a 64-row reduction tile is
group_stride = 64 * N / group_sizescaleentries — advancing by
n_groups *that (4N for gs=32) reads out of boundsfrom the second K-tile on. Result:
quantized_matmul(transpose=False)withgroup_size=32produced garbage (and NaN under some inputs) on Metal for anyK > 64.K = 64(a single reduction tile) was correct — which is why thebug was localized only after comparing K=64 vs K=128.
group_size64/128 andtranspose=Truewere unaffected (they use thegeneric loader / the
reduction_dim == 1branch respectively).The same over-advance existed in the fp (fp8/MX) loader and is fixed here too.
Fix
Advance by
group_stride(notn_groups * group_stride) in thereduction_dim == 0branch of both loaders.Tests / verification
test_qmm_non_transposed_group_size_lt_64: gs=32, bits2/4/8, M 8/33/65, K 64 (single-tile control) / 128 / 256 (multi-tile).
Fails on main at K>64 (garbage/NaN), passes with the fix.
test_quantizedon a local Metal build (M5 Max, macOS 27, Xcode 27,mlx from source): previously 48 failed subtests -> now 37 passed,
3106 subtests passed.
~1e-2) for all bits/gs, bothtranspose flags, and odd-M partial tiles.