Fix QuantizedBlockLoader::load_safe bounds check comparing wrong field - #4203
Closed
katnoria wants to merge 1 commit into
Closed
Fix QuantizedBlockLoader::load_safe bounds check comparing wrong field#4203katnoria wants to merge 1 commit into
katnoria wants to merge 1 commit into
Conversation
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.
Member
|
There are already quite a few PRs on this. |
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.
Summary
QuantizedBlockLoader::load_safe'sreduction_dim == 1branch(
mlx/backend/metal/kernels/quantized.h,load_safe) checksbiindexesBROWS— the loader's output axis (Mfor the activationloader,
Nforqmm_t's weight loader), not the reduction axis. Everycall site passes the valid-
BROWScount insrc_tile_dim.y, byconvention:
qmm_t_implcalls withshort2(BK, num_outs),affine_gather_qmm_rhswithshort2(k_remain, tgp_bn). Both put theBROWS-count in.y. The guard compares against.xinstead — theBCOLS/K-tile-width count.Why this was never observed
Every
qmm_t/gather_qmm_tinstantiation shipped to date hasBROWS == BK == 32(the loader's tile is always square in thatdimension), so
.x == .yin every existing call and the miscomparedfield 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 thanK-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.yinstead of.x. Zerobehavior change for every existing
BROWS==BKinstantiation (confirmed:.x == BROWSthere too, so the two fields were interchangeable inexactly 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 frompython/tests/): 34/34pass, both
MLX_METAL_JIT=ONand the defaultMLX_METAL_JIT=OFFbuild —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_ttile-sizedispatch fix) that is what actually exercises the
BROWS != BKpath andis what surfaced this bug — see #4204 — but this fix stands on
its own and is correct independent of whether that change lands.