Skip to content

Fix and enable non-transposed NAX qmm - #4051

Merged
zcbenz merged 4 commits into
ml-explore:mainfrom
gordofreemo:fix-nax-non-transposed-qmm
Aug 12, 2026
Merged

zcbenz merged 4 commits into
ml-explore:mainfrom
gordofreemo:fix-nax-non-transposed-qmm

Conversation

@gordofreemo

Copy link
Copy Markdown
Contributor

What this does

qmm() only lets NAX through when transpose == true, so affine_qmm_n_nax is compiled but never actually run and is currently unreachable. It has two separate problems, both of which this PR fixes, and then enables the path to be reachable.

FIX: Weight addressing uses the transposed layout

qmm_n_nax_tgp_impl uses the same QuantizedBlockLoader instantiation as the
generic qmm_n_impl in kernels/quantized.h:

QuantizedBlockLoader<T, BK, BN, BN_padded, 0, WM * WN * SIMD_SIZE, group_size, bits>

but set the pointers up for w = [N, K] rather than [K, N]:

qmm_n_impl (correct) qmm_n_nax_tgp_impl (before)
weight offset wl += y_col * bytes_per_pack / pack_factor wl += y_col * K_w
scales offset scales += y_col / group_size scales += y_col * K_g
biases offset biases += y_col / group_size biases += y_col * K_g
leading dim loader_w(..., N, ...) loader_w(..., K, ...)
implied layout w = [K, N], grouped along N w = [N, K], grouped along K

This fix makes those four lines match qmm_n_impl. affine_gather_qmm_n_nax shares the impl, so this fixes that too.

FIX: No partial M tile handling

This same function had (void)M;, its bounds line commented out, and called Atile.load / Dtile.store unconditionally. qmm_nax() sizes the grid with (M + bm - 1) / bm, so partial M tiles are dispatched and any M % 64 != 0
read x and wrote y out of bounds.

Fixed by porting what qmm_t_nax_tgp_impl already does: sgp_sm plus a dispatch_bool picking load_safe/store_safe. That split is compile time, so the aligned path keeps the original unguarded loads and costs nothing, and only one branch is instantiated per call so the x += BK walk isn't duplicated. sgp_sm is min(int(SM), M - (y_row + tm)) rather than truncating the distance to short, matching the int16 fix already made for the transposed kernel.

FIX: The dispatch guard

if (metal::is_nax_available() && (transpose || (N % 64 == 0)) &&
    (K % 64 == 0) && ...

The non-transposed weight tile is BK x BN over w = [K, N] and the loader isn't clamped in N, so it also needs N % 64 == 0. Affine quantization with group_size >= 64 gives that structurally, since the groups run along N. This only rejects group_size == 32 with N not a multiple of 64, which falls back to the generic kernel like before.

Scope: this patches qmm() only. gather_qmm() stays gated on transpose.

Tests

test_qmm_non_transposed in python/tests/test_quantized.py:

  • transformer sized K/N: (2048, 2048), (512, 2048), (2048, 512), (11008, 2048)
  • M aligned and unaligned: 1, 2, 31, 32, 33, 63, 64, 65, 96, 97, 100, 127, 128, 129, 250, 256. The 33 to 63 range is where a whole simdgroup of the M tile lands past the end of the matrix
  • batched x, 3D and 4D, with unaligned M
  • M = 33000 with a partial M tile, for the int16 distance case
  • group_size 64 and 128, bits 4 and 8

Tolerances and the 1/sqrt(K) operand scaling follow the existing test_qmm. Worth noting the existing test_qmm already covers transpose=False at M in {8, 32, 33, 64} with N, K in {128, 256}.

Validation

I don't have M5 hardware. All NAX validation was on an iPhone 17 Pro (A19 Pro), iOS 26.5.2, through the Swift bindings, against a dequantized reference: 52 shape/M/batch cases covering every unaligned M above plus batched. Before the fix, relative error ran 1.5 to 7826, with an output norm of 427,429 against an expected 24,009 at K=N=2048, M=500. After, it's bit exact against the reference on that device.

On the M3 I have, is_nax_available() is false, so the full test_quantized.py suite passes there only through the generic path. That confirms no regression on the fallback, not the NAX path. The kernel itself does compile (quantized_nax.air builds clean).

Performance

Same iPhone 17, but this time running LoRA fine tuning on SmolLM3-3B 4-bit (group_size 64, r=8, batch 1).

One full training run with 405 examples and 1215 iterations. It took 2.9 hours before the fix and 1.5 after the fix (~1.93x speedup). Final loss 0.8829 with the fix against 0.8813 without, a 0.18% difference, and the two loss traces correlate at 0.9999 across 1210 steps.
naxab_e2e_2026-08-07

A paired per op benchmark on fixed synthetic shapes puts the backward phase at 1.65 to 2.13x and the whole iteration around 1.55x.
naxab_speedup_2026-08-06

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

gordofreemo and others added 2 commits August 7, 2026 12:59
qmm() only lets NAX through when transpose is true, so affine_qmm_n_nax
never actually runs and nothing in CI touches it. Two things are broken
in it.

The weight addressing is set up for [N, K]: it offsets by the K_w/K_g
row strides and passes K as the leading dim. With transpose=false the
operand is [K, N], grouped along N. The loader instantiation is already
the same one the generic qmm_n_impl uses, so the offsets just need to
match it. Same impl backs affine_gather_qmm_n_nax, so that gets fixed
as well.

It also ignores M entirely. The bounds line is commented out and the
tile load/store are unguarded, but the grid is ceil(M/BM), so partial
tiles do get dispatched and anything not a multiple of 64 writes past
the end of y. Pulled sgp_sm + dispatch_bool + load_safe/store_safe over
from qmm_t_nax_tgp_impl. The split is compile time so the aligned path
doesn't change. Used min(int(SM), ...) rather than truncating the
distance to short, same as the int16 fix over there.

The guard now also needs N % 64 == 0 for the non-transposed case since
the weight tile isn't clamped in N, but any group_size >= 64 gives that
anyway. Left gather_qmm() gated since I haven't tested that dispatch
path.

Test covers unaligned M, mainly 33..63 where a whole simdgroup ends up
past the end of the matrix, plus batched x and a large-M partial tile.
@zcbenz zcbenz added the await verification This pull request is non-trivial and requires a human expert to verify its correctness. label Aug 8, 2026
threadgroup_barrier(mem_flags::mem_threadgroup);
loader_w.load_unsafe();
threadgroup_barrier(mem_flags::mem_threadgroup);
dispatch_bool(!is_unaligned_sm, [&](auto kAlignedM) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispatch_bool is useful when you have to call some function that requires a template argument, in this case you should just do normal if:

if (sgp_sm == SM) {
  load()
} else {
  load_safe()
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks, switched to a plain if (sgp_sm == SM) for both the load and the store.

@zcbenz zcbenz added await response This pull request is waiting for response from the author. and removed await verification This pull request is non-trivial and requires a human expert to verify its correctness. labels Aug 10, 2026
dispatch_bool was doing nothing here — the two branches only pick between
Atile.load/load_safe and Dtile.store/store_safe, neither of which takes a
template argument, so the compile-time constant was unused. Replaced with a
normal if on sgp_sm == SM.

Also drops the barriers out of the divergent region. sgp_sm varies across
simdgroups in a threadgroup (tm is 0 or SM), so on a partial tile the two
dispatch_bool branches were taken by different simdgroups with the k-loop
barriers inside each. The if now wraps only the load and the store, and every
threadgroup_barrier sits in uniform control flow.

Behaviour is unchanged: same predicate, same two calls, and the branch is
simdgroup-uniform and loop-invariant.
@zcbenz zcbenz removed the await response This pull request is waiting for response from the author. label Aug 11, 2026

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@zcbenz
zcbenz merged commit d04e5db into ml-explore:main Aug 12, 2026
@gordofreemo
gordofreemo deleted the fix-nax-non-transposed-qmm branch August 17, 2026 07:02
@BrewTestBot BrewTestBot mentioned this pull request Aug 18, 2026
1 task done
gordofreemo added a commit to gordofreemo/mobile_LoRA_ft that referenced this pull request Oct 5, 2026
The Xcode project references ../mlx-swift as a local Swift package, but the
directory was never committed, so a fresh clone could not build the app.
VENDORED.md records the upstream base and LOCAL_PATCHES.md the kernel
patch, which is upstream since ml-explore/mlx#4051. MIT, as upstream.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants