Fix fp quantized matmul corruption when the quantized dim is not a multiple of 32 - #3912
Fix fp quantized matmul corruption when the quantized dim is not a multiple of 32#3912kapellirohith wants to merge 1 commit into
Conversation
|
Coordination note from M5/NAX testing in #3922: This PR remains the authoritative fix for NVFP4's legal 16-wide tails and the required plain-kernel bounded load/store work. I deliberately did not duplicate those changes. The independent M5 matrix clarified the merge interaction:
Independent native M5 tests for #3922 kept affine/MXFP4/MXFP8 tail errors below 4.7e-4; untouched NVFP4 non-64 K remains failing, which confirms the scope split rather than contradicting this PR. Security scope for the related reports: the observed behavior is same-process incorrect tensor values and, in #3856, same-process allocator-pool reuse. We found no cross-process disclosure, arbitrary code execution, sandbox escape, or other trust-boundary crossing; current evidence supports numerical correctness bugs, not cybersecurity vulnerabilities. |
|
Thanks for the heads up, and for running this on M5. That's the piece I couldn't test. Scope split looks right to me. This PR fixes the plain kernels for NVFP4 dims that are We both touch quantized.cpp and test_quantized.py so there's a conflict either way. |
|
Thanks — this confirms both the scope split and the observed M5 NVFP4 path. The cleanest sequence is #3922 first, then rebase #3912 as offered: #3922 supplies the sorted |
| loader_w.next(); | ||
| } | ||
| } | ||
| if (num_k > 0) { |
There was a problem hiding this comment.
You can putting this inside a if constexpr (group_size == 16) to avoid overhead on other quantization modes.
| threadgroup_barrier(mem_flags::mem_threadgroup); | ||
| loader_x.load_safe(short2(BK, num_els)); | ||
| loader_w.load_unsafe(); | ||
| loader_w.load_safe(short2(num_outs, BK)); |
There was a problem hiding this comment.
Similarly here if constexpr (group_size == 16) can be used to choose load_unsafe for faster load.
| // instead of reading past the source (partial tiles happen when the | ||
| // quantized dim is not a multiple of the block, e.g. nvfp4's | ||
| // group_size=16). | ||
| if (bi >= src_tile_dim.y || bj * pack_factor >= src_tile_dim.x) { |
There was a problem hiding this comment.
Should the condition depend on reduction_dim, i.e. bi >= src_tile_dim.y when reduction_dim == 0, and bi >= src_tile_dim.x when reduction_dim == 1?
| const Stream& s, | ||
| const std::string mode) { | ||
| if (metal::is_nax_available() && transpose && | ||
| if (metal::is_nax_available() && transpose && (K % 64 == 0) && |
There was a problem hiding this comment.
I'm not sure on this, should drop it if you can not test it.
9c8d78c to
3cd6f4a
Compare
|
All four addressed, rebased on main.
Same on the qmm_n side, with Whether the mask should depend on The same reasoning is why the NAX dispatch guard. Dropped, as asked. Revalidated on the result: three reproducers clean, 289/289 misalignment sweep, 4515 randomized differential cases across all modes, bit widths and dtypes with no failures, |
…ltiple of 32 nvfp4's group size of 16 is the only mode that lets the quantized dim of a matmul legally be a multiple of 16 but not of 32, and the fp quantized Metal kernels tile that dim by 32 without bounding the tail: - fp_qmm_t_impl looped over K_eff in full-BK steps with unbounded loads, so the final iteration read 16 columns of the next row's weights and scales (past the buffer for the last row) into the accumulator. - fp_qmm_n_impl stored full 32-column tiles for a 16-column tail; the spilled columns land on the next output row, racing the threadgroup that owns it (results differ run to run), and past the end of the output buffer for the last row. - QuantizedBlockLoader::load_safe compared the row index bi against the column bound src_tile_dim.x, so the K-remainder step of fp_gather_qmm_rhs zeroed the threads owning output columns 16..31 of each tile and silently dropped the last 16 K-elements from them. Bound the K loop of fp_qmm_t_impl with a remainder tile, clip the w loads and the stores of fp_qmm_n_impl to the valid columns, and make load_safe mask rows against .y and packed columns against .x, matching the convention the steel BlockLoader already uses. The row half of that mask is not nvfp4 only. With reduction_dim == 1 the old compare was bi >= src_tile_dim.x, that is bi >= BK == 32, while bi ranges over [0, BROWS) == [0, 32), so it never fired and any N that is not a multiple of BN read weight and scale rows past N. The store already clipped those columns so results were correct, but the read was out of bounds, and affine and the mxfp modes reach it too. The affine loader therefore gets the same change. A quantized dim is always a multiple of group_size, so it can only end in a partial tile when group_size does not divide the block. Only nvfp4 does; mxfp4 and mxfp8 use group_size 32 and every affine group size is a multiple of the block. The remainder tile and the column bound are selected with if constexpr so they are discarded at compile time for those modes, which keep load_unsafe on the aligned path.
3cd6f4a to
3a755bf
Compare
|
Hey, please see #4009. Thanks! |
Problem
nvfp4 is the only quantization mode whose group size (16) lets the quantized
dimension of a matmul legally be a multiple of 16 but not of 32.
mx.quantize(w, mode="nvfp4")acceptsK = 1040, and the fp quantized Metalkernels tile that dimension by 32 without bounding the 16-wide tail. Whenever it
happens, every matrix-sized
quantized_matmul/gather_qmmon the GPUsilently returns corrupted results. The CPU backend and the vector (decode)
kernels handle the same shapes correctly, so a model can decode perfectly and
corrupt during prefill, which is about as quiet as corruption gets.
Minimal reproducer (M3 Pro, macOS 25.5, main @ 39d9a8a):
stream=mx.cpu: 1.2e-3.M=1(vector kernel): 1.1e-3. AlignedK=1024: 3e-3.Only the GPU matrix path with
K % 32 == 16is wrong.The three defects
All in
mlx/backend/metal/kernels/fp_quantized.h, introduced with the Metalnvfp4 support in #2946.
fp_qmm_t_implran its K loop pastK_eff(for (int k = 0; k < K_eff; k += BK)with full-BK loads), so the final iteration read 16 columns of thenext output row's packed weights and scales (past the buffer for the last
row) into the accumulator. Metal shader validation on the unpatched kernel:
Invalid device load at offset 2129920 … "nvfp4_qmm_t_float16_t_gs_16_b_4_alN_true_batch_0"(the weight buffer is exactly 2129920 bytes). Also backs the tiled
gather_qmm, so the unsorted MoE path fails identically.fp_qmm_n_implstored full 32-column tiles for a 16-column tail. Thestore clipped rows only (
store_result_safe(y, N, short2(BN, num_els))), sowith
N % 32 == 16the last column tile stored 32 columns into rows only Nlong: the 16 spilled columns land on the next row's first 16 outputs (racing
the neighbouring threadgroup) and past the end of the output buffer for the
last row. Unpatched: 200 identical runs give 61 distinct results,
corruption sits exactly at columns 0 to 15, and shader validation flags
Invalid device store at offset 1064976 … "nvfp4_qmm_n_float16_t_gs_16_b_4_batch_0".QuantizedBlockLoader::load_safemasked the wrong axis (if (reduction_dim == 1 && bi >= src_tile_dim.x)):biis the thread's row,src_tile_dim.xthe column-direction bound. In the K-remainder step of thesorted-MoE kernel
fp_gather_qmm_rhs(tile_w = short2(k_remain=16, tgp_bn)), this zeroed the threads owning output columns 16 to 31 of each tileand silently dropped their last 16 K-elements. Positive ID: on the unpatched
kernel the wrong columns are exactly those ≡ 16..31 (mod 32), and their
values equal
ref - x[:, K-16:] @ w[e][:, K-16:].Tto within 0.05 (fp16rounding): the dot product minus exactly its tail. In the
fp_qmm_tcallsites the same compare is
bi >= 32, dead code, so their intended columnmasking never ran.
Fix
fp_qmm_t_impl: iterateK_eff / BKfull tiles, then one boundedremainder tile, mirroring
fp_qmm_n_impl's existing K handling. The split-Kkernel
fp_qmm_t_splitkpassesK_eff = k_partition_size, which Fix incorrect nvfp4 quantized_matmul through the split-K path #3854 keepsa multiple of 32, so
num_k == 0and split-K is unchanged.fp_qmm_n_impl: takenum_outs = min(BN, N - y_col), clip the w loads toit, and store with
store_result_safe(y, N, short2(num_outs, num_els)).Removes the race and the out-of-bounds store.
QuantizedBlockLoader::load_safe(fp_quantized.hand the affine twin inquantized.h): mask rows against.yand the thread's columns against.x.A thread owns
n_reads * pack_factorcontiguous columns and every legalpartial extent is a whole number of quantization groups, so a thread is never
straddled; the new
static_assert(group_size % (n_reads * pack_factor) == 0)in both loaders pins that invariant.
A quantized dim is always a multiple of
group_size, so it can only end in apartial tile when
group_sizedoes not divide the block. Only nvfp4 does;mxfp4 and mxfp8 have
group_size == 32 == BK == BN, and every affine groupsize is a multiple of the block. The remainder tile and the column bound are
therefore selected with
if constexprongroup_size % BK != 0,group_size % BN != 0andgroup_size % BCOLS != 0, so they are discarded atcompile time for those modes.
This also removes an out-of-bounds read that affine and mxfp hit today
The row half of the mask is not nvfp4-only. In
qmm_t_implthe w loader isinstantiated with
BROWS = BNandreduction_dim = 1, and the call isload_safe(short2(BK, num_outs)). The old compare wasbi >= src_tile_dim.x,that is
bi >= BK == 32, whilebiranges over[0, BROWS) == [0, 32), so itnever fired: whenever
N % BN != 0the last tile read weight and scale rowspast
N. The store already clipped those columns, so results were correct;this is a read past the end of the buffer, not corruption. It reproduces on
affine, which is the most used mode:
K = 32768makes the packed weight buffer an exact multiple of the page size,so the over-read crosses the allocation. Under Metal shader validation on
39d9a8a:
With this change the same run is clean, as are the two nvfp4 shapes below. The
same applies to mxfp4 and mxfp8 with
N % BN != 0.On the loader mask:
biis a row index andbja packed column index, andevery call site passes
src_tile_dimas (valid columns, valid rows), swappingwhich physical dim that is to match the layout.
fp_qmm_t_implpassesshort2(BK, num_outs)withBROWS = BN,fp_qmm_n_implpassesshort2(num_outs, BK)withBROWS = BK, andfp_gather_qmm_rhspicksshort2(k_remain, tgp_bn)orshort2(tgp_bn, k_remain)ontranspose. That isthe convention steel's
BlockLoader::load_safealready uses(
src_tile_dim - short2(bj, bi)), so the row index tests against.yin everycase and the mask does not depend on
reduction_dim.Validation
M3 Pro (applegpu_g15s), macOS 25.5, against main @ 39d9a8a. "ref" is fp64 on
the MLX-dequantized weights (bit-identical CPU vs GPU dequantize, asserted).
fp_qmm_t_splitk), K∈{2048,4096,8192}N % BN != 0The campaign's CPU cross-check uses a looser gate for fp16 and bf16, since the
CPU backend accumulates those differently over a large K; that is not code this
PR touches.
The added tests use M ≥ 33;
get_qmv_batch_limitcaps the vector/matrixthreshold at 32, so the tiled kernels are exercised on every Apple GPU family
(M1 to M4, including Max/Ultra). 17 of the 19 subtests fail on 39d9a8a and all
pass with the fix. The two that pass on both are controls: the vjp with
transpose=Falsehas an aligned quantized dim, and the sorted gather withtranspose=Falseruns the loader withreduction_dim == 0, where the old rowcompare was already correct.
To confirm the
if constexprguards cost the other modes nothing, I compiledfp_quantized.metalagainst the base headers and against this branch andcompared every emitted kernel body after normalizing metadata ids. Of the 306
gs_32kernels, 253 are byte-identical and none gained a singleinstruction; 53 shrank, for a net of −141 instructions, while the +12950
instructions added by the fix land entirely on the nvfp4 kernels. The 53 that
shrank are exactly the
reduction_dim == 1families (qmm_t,qmm_t_splitk,gather_qmm_t,gather_qmm_rhs_nt), where the row mask now short-circuits therows it used to read out of bounds.
Perf (aligned fast path, min-of-medians, ms, before -> after)
Nothing regresses. The mxfp4, mxfp8 and affine rows are the ones that matter
for the
if constexprguards above, and the unaligned gather rows are thekernels where the row mask now short-circuits work the base build was doing on
rows it read out of bounds.
Why fix the kernel rather than reject the dimension
group_size = 16advertises that quantized dimensions which are multiples of 16are supported; silently corrupting a legal, documented input is an API-contract
violation the kernel should honor rather than an input to reject at the op layer.
The inconsistency already shows:
mx.quantize(w, mode="nvfp4", stream=mx.cpu)throws a reshape error when
w.size % 32 != 0while the GPU path accepts thesame tensor and later corrupts. One backend rejects what the other silently
mishandles. Making the kernels correct for every legal group_size=16 dimension
resolves both.
Provenance
Introduced with the Metal nvfp4 kernels in #2946. #3854 narrowed only the
split-K route: for
K ≡ 16 (mod 32)no split_k makesK % (split_k * 32) == 0,so those shapes always take its
split_k = 1fallback straight into plainqmm()->fp_qmm_t_impl, the unbounded loop fixed here. The reproducer above(M=50, K=1040, B=1, transpose=True) is exactly that path, so the shapes #3854
meant to protect were still corrupted.
Related but distinct: #3856 reports affine
gather_qmmcorruption at large unalignedrow counts (
n > 32768,n % 64 != 0) on M5, a row-count (M) bug. This is aquantized-dimension (K/N) bug specific to nvfp4; #3856's own repro does not reproduce
against this branch, and this fix leaves its
K % 64 == 0shapes on the same NAXkernel, so the two do not overlap.
Tests
test_fp_qmm_non_multiple_of_32covers K=1040/528 (transpose=True) and N=1040(transpose=False, run twice and compared bit for bit to catch the store race),
each in fp32, fp16 and bf16, plus mxfp4 and mxfp8 at the same block-unaligned
shapes as a control that the
if constexprguards did not change them.test_fp_gather_qmm_non_multiple_of_32covers sorted and unsortedgather_qmmin the same three dtypes, with the tail on K (transpose=True) and on N
(transpose=False).
test_fp_qmm_non_multiple_of_32_vjpcovers the backward ofboth transposes. Inputs are scaled by
1 / sqrt(K)so the dot products areO(1); at magnitude 32 a single fp16 ulp is 0.03 and would swamp the tolerance.