[HIP] Fix rmsnorm_quant cross-row reads and writes for rows not a multiple of 4 bytes - #5290
[HIP] Fix rmsnorm_quant cross-row reads and writes for rows not a multiple of 4 bytes#5290rk9595 wants to merge 1 commit into
Conversation
add_rmsnorm_quant_kernel rounded every gmem descriptor up to a whole dword, so any row whose byte length is not a multiple of 4 -- fp16/bf16 with odd n, or fp8/int8 output with n % 4 != 0 -- had the first element(s) of the *next* row inside its window. Those elements were read into the sum of squares, and the block also stored its own normalized values over them, racing with the block that owns that row; that race is why only some rows show corruption and which ones varies. The same round-up read past the end of the tensor on the last row, and past the end of `weight` on every row. Bound each descriptor at the row's exact byte length, and redo the row's trailing sub-dword elements through single-element load/store. Those are b16/b8 accesses that fit inside the exact bound, so they are performed whether the hardware range-checks a buffer access per byte or per dword, and the fix does not depend on which. Aligned rows are unchanged by construction: the exact bound equals the old rounded one and both tail loops are skipped on a uniform branch. Add unaligned-n coverage with guard rows to both rmsnorm test files. The existing sweeps only ever ran dword-aligned n, which is why this survived. Verified on gfx942 (MI325X): the issue's repro goes from max_abs_diff 0.511719 to exact, both test files pass, and perf is within 0.5% both directions on 4096x4096 and 8192x8192. Fixes ROCm#5044 Signed-off-by: Rakesh Kariya <rakesh.kariya@somaiya.edu>
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
PR title tags & labels: |
|
hidden_size=769: this size will in real model? |
|
No — I don't know of a production model with an odd hidden size. Hidden dims are multiples of 64 in practice, and the reporter picked 769 as the smallest odd size next to an aligned one, so this is not on any hot path. What made me write the fix is the blast radius rather than the shape. If you'd rather not carry the kernel change for a shape real models don't use, two smaller versions:
My preference is the full fix, then exact-bounds-only, but I'm happy to cut it down to whichever you want — say which and I'll push. One other thing: the workflows on this PR are still sitting at |
Summary
Fixes #5044.
add_rmsnorm_quant_kernelbounds each of its gmem descriptors atnrounded up to a whole dword:Rows are contiguous, so for any row whose byte length is not a multiple of 4 that window reaches into the next row. With one block per row and
n = 769fp16:normalize(next_row[0]) * weight[769]over it — racing with the block that actually owns that row. Whichever store retires last wins, which is why only some rows show corruption and which ones varies between runs.The same round-up reads past the end of the input tensor on the last row, and past the end of
weighton every row.This is not only the reported fp16 path.
ooba_o = 4 / sizeof(DTYPE_O_STORE)is 4 for fp8/int8 output, sormsnorm_quant/add_rmsnorm_quantclobber up to 3 bytes of the next row whenevern % 4 != 0, andresidual_outinadd_rmsnorm_quanthas the 2-byte version of the same problem. Same class as #4467, which fixed it for the packed fp4 output only.The existing sweeps run
n in [4096, 8192, 16384, 32768, 65536]and[1024, 2048, 3584, 4096, 8192]— all dword-aligned — which is why this survived.Technical details
row_bytes_i/row_bytes_o). fp4 keeps(n + 1) / 2, which was already exact. Reads past the row return 0, contributing nothing to the sum of squares or the abs-max; writes past it are dropped.opus::load<1>/opus::store<1>. Those areb16/b8accesses that fit inside the exact bound, so they are performed whether the hardware range-checks per byte or per dword — the fix does not depend on which, and needs no per-arch probe.column_of(slot)maps a register slot back to its row column, mirroring the chunking inload_vector_nbytes/store_vector(num_load_instreaches the store asnum_repeat, so one mapping serves both). The quant tail reusesscaled_caston a 2-lane vector and keeps lane 0, so its rounding is identical to the vector path rather than a reimplementation.row_bytesequals the old rounded bound, the descriptors are bit-identical, and both tail loops are skipped on a uniform branch.Test plan
op_tests/test_rmsnorm2d.py: newtest_rmsnorm2d_unalignedandtest_rmsnorm2d_fuseAdd_unalignedwith a guard-row helper, overn in [769, 1023, 2047, 4095, 6143, 8191](one per dispatch bin; 769 is the issue's own shape) xm in [1, 7, 33]x {fp16, bf16}. Values go throughtorch.testing.assert_close, since the cross-row write lands inside the tensor and a guard-row check alone would pass it; the guard rows catch the separate write past the last row. These are separate cases rather than additions tol_n, to keep the file's CI time.op_tests/test_rmsnorm2dFusedAddQuant.py: generalized fix(module_rmsnorm_quant): bound packed FP4 output stores #4467's fp4-only guard to every kernel-written output (plain, quant, andresidual_out), addedn = 1027and2050to the sweep, and made group quant skip shapes it cannot express so thosendo not break modes 7/8.Test result
gfx942 (MI325X), inside
rocm/pytorch:rocm7.2.4_ubuntu22.04_py3.10_pytorch_release_2.10.0.The issue's script verbatim:
(The corrupted row indices differ from the ones in the issue and move run to run, as expected for a race; column 0 and the 0.51 magnitude reproduce exactly.)
Both test files pass, including all 36 new unaligned cases. On
test_rmsnorm2dFusedAddQuant.py(modes 1/2/5/6, fp8) thecheckAllclosewarning rate is identical before and after the patch at 2.0 per shape with no hard failures, so this introduces none — those are pre-existing bf16 tolerance noise on aligned shapes.Perf, median of 7 reps x 200 iters, bf16, microseconds:
Under half a percent, both directions. (m=256 shapes were also measured but are launch-overhead bound — 256x4096 and 256x8192 cost the same despite double the work — so their deltas are process noise.)
I only had gfx942; CI covers gfx950.
If you would rather keep this minimal, the exact-bound change alone fixes the reported corruption and I am happy to drop the tail path — I kept it because it makes the kernel correct regardless of the hardware's range-check granularity, but it is a clean separation if you prefer.