Skip to content

Fix QuantizedBlockLoader::load_safe bounds check comparing wrong field - #4203

Closed
katnoria wants to merge 1 commit into
ml-explore:mainfrom
katnoria:qmm-load-safe-fix
Closed

Fix QuantizedBlockLoader::load_safe bounds check comparing wrong field#4203
katnoria wants to merge 1 commit into
ml-explore:mainfrom
katnoria:qmm-load-safe-fix

Conversation

@katnoria

@katnoria katnoria commented Aug 12, 2026

Copy link
Copy Markdown

Summary

QuantizedBlockLoader::load_safe's reduction_dim == 1 branch
(mlx/backend/metal/kernels/quantized.h, load_safe) checks

if (reduction_dim == 1 && bi >= src_tile_dim.x) {

bi indexes BROWS — the loader's output axis (M for the activation
loader, N for qmm_t's weight loader), not the reduction axis. Every
call site passes the valid-BROWS count in src_tile_dim.y, by
convention: qmm_t_impl calls with short2(BK, num_outs),
affine_gather_qmm_rhs with short2(k_remain, tgp_bn). Both put the
BROWS-count in .y. The guard compares against .x instead — the
BCOLS/K-tile-width count.

Why this was never observed

Every qmm_t/gather_qmm_t instantiation shipped to date has
BROWS == BK == 32 (the loader's tile is always square in that
dimension), so .x == .y in every existing call and the miscompared
field was interchangeable with the correct one — the check has been a
no-op, correct "by luck," not by construction. It stops being a no-op the
moment a caller sets BROWS != BK — e.g., a wider output tile than
K-tile — at which point some in-range output rows fall on the wrong side
of the miscompared bound and get force-zeroed by an unrelated check
instead of the intended rows.

The fix

One-line change: compare against src_tile_dim.y instead of .x. Zero
behavior change for every existing BROWS==BK instantiation (confirmed:
.x == BROWS there too, so the two fields were interchangeable in
exactly that case) — this is a pure latent-bug fix, not a behavior change,
for the kernel configurations that ship today.

Testing

python -m unittest test_quantized -v (run from python/tests/): 34/34
pass, both MLX_METAL_JIT=ON and the default MLX_METAL_JIT=OFF build —
output-identical before/after, as expected for a fix that's a no-op on
every shipped instantiation. Also verified by a broader stress sweep
(~550 shape/dtype/group_size/bits combinations against a
dequantize+dense-matmul reference) with no behavior change relative to
unmodified upstream/main.

This is being split out from a separate, larger change (a qmm_t tile-size
dispatch fix) that is what actually exercises the BROWS != BK path and
is what surfaced this bug — see #4204 — but this fix stands on
its own and is correct independent of whether that change lands.

In the reduction_dim == 1 branch, `bi` (which indexes BROWS -- the
output axis, N for qmm_t's weight loader) was compared against
src_tile_dim.x. Every call site actually passes the valid-BROWS-count
in src_tile_dim.y by convention (qmm_t_impl's `short2(BK, num_outs)`,
affine_gather_qmm_rhs's `short2(k_remain, tgp_bn)`), so this was
comparing a row index against a K-tile-width instead of against its
own valid-row count.

This has been a silent no-op in every kernel instantiated to date,
because every existing qmm_t/gather_qmm_t config happens to have
BROWS == BK == 32, so `bi < BROWS` already implied `bi < BK` and the
check never fired -- correct output "by luck": the rows it should
have zero-padded were already beyond `num_outs` and discarded by
store_result_safe regardless of what stale/adjacent memory they read
into threadgroup memory first.

It stops being a no-op once BROWS is parametrized independently of
BK (an upcoming change needs exactly that), at which point some
still-valid output rows fall on the wrong side of the miscompared
bound and get forced to zero instead of the safe/discarded ones,
producing wrong results at partial-tile N boundaries.

Fix: compare against src_tile_dim.y instead. Zero behavior change
for every existing BROWS==BK instantiation (.x == BROWS there too,
so the two fields were interchangeable in exactly that case);
correctness bug fix once BROWS != BK.
@zcbenz

zcbenz commented Aug 12, 2026

Copy link
Copy Markdown
Member

There are already quite a few PRs on this.

@zcbenz zcbenz closed this Aug 12, 2026
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