[Bugfix] Fix two A-tile loading bugs in the preshuffle GEMM - #1007
Open
JohnQinAMD wants to merge 1 commit into
Open
[Bugfix] Fix two A-tile loading bugs in the preshuffle GEMM#1007JohnQinAMD wants to merge 1 commit into
JohnQinAMD wants to merge 1 commit into
Conversation
Both bugs are in how the A tile reaches and leaves LDS. Neither produces an
error today: the first returns wrong results, the second returns correct
results slowly.
1. Reject tiles whose A tile is only partially loaded.
num_a_loads is a truncating division, so tile shapes whose per-thread A copy
does not divide into whole 16B loads never fetch the tail of the A tile; the
kernel then computes on stale LDS and returns wrong results at full speed
with no diagnostic:
bytes_per_thread_a = (tile_m * tile_k * elem_bytes) // total_threads
num_a_loads = bytes_per_thread_a // a_load_bytes # remainder dropped
The requirement is tile_m * tile_k * elem_bytes % 4096 == 0. For bf16 at
tile_k=64 that makes tile_m a multiple of 32, so tile_m of 112/144/176/208
load only 48/64/80/96 of the 56/72/88/104 bytes per thread they own. This is
reachable by accident because _TILE_PRELOAD_TABLE advertises (48, 64, 128),
(80, 128, 256) and (112, 64, 256) as tuned entries.
Validate the exact condition on the tile size rather than on either
truncated intermediate.
2. Restore per-k-step A-fragment LDS reads in mma_kloop.
#974 replaced the per-ki copies with a single whole-tile fx.copy. The two are
semantically identical, but the single copy leaves the scheduler no room to
interleave the reads with the MFMAs that consume them. Measured on MI355X
(gfx950), bf16 M=54560 K=8192 N=8192, sync path, single-hunk A/B on ac227c3:
tile_m x tile_n x tile_k whole-tile per-k-step
128 x 256 x 256 303 1011 3.3x
128 x 128 x 256 687 774 1.13x
64 x 256 x 256 863 859 neutral
128 x 256 x 128 1106 1103 neutral
128 x 256 x 64 1388 1381 neutral
The regression is confined to specific large-tile_k shapes; everything else
is within the ~1% run-to-run noise (128x256x64 is the control here, and the
affected cell is stable to +-0.3% over five repeats).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes two correctness/performance issues in the preshuffle GEMM kernel’s A-tile movement through LDS: (1) prevents silently wrong results when the A tile cannot be evenly distributed into 16B per-thread loads, and (2) restores per-k-step A-fragment LDS reads to enable better MFMA/read interleaving for large tile_k shapes.
Changes:
- Add a compile-time validation in
compile_preshuffle_gemmthat rejects(tile_m, tile_k, dtype)combinations leaving a partial per-thread 16B A load (previously computed on stale LDS). - Change
mma_kloopto issue A-fragment LDS reads per k-iteration instead of one whole-tile copy, improving scheduling latitude and addressing the reported bf16tile_k=256slowdown. - Add unit tests asserting rejection/acceptance of representative
tile_mvalues for bf16tile_k=64.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
kernels/gemm/preshuffle_gemm.py |
Adds strict A-tile load-granularity validation and switches A-fragment reads back to per-k-step copies for better MFMA interleaving. |
tests/kernels/test_preshuffle_gemm.py |
Adds compile-time tests ensuring partial A-tile load shapes raise ValueError and whole-load shapes still compile. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
Two bugs in how the A tile reaches and leaves LDS in
preshuffle_gemm. The first returns wrong results, the second returns correct results 3.3× slower on some shapes.Bug 1 — tiles whose A tile is only partially loaded
num_a_loadsis a truncating division, so tile shapes whose per-thread A copy does not divide into whole 16B loads never fetch the tail of the A tile. The kernel then computes on stale LDS and returns wrong results at full speed with no diagnostic:The requirement is
tile_m * tile_k * elem_bytes % 4096 == 0. For bf16 at tile_k=64 that makestile_ma multiple of 32:Easy to hit by accident:
_TILE_PRELOAD_TABLEadvertises(48, 64, 128),(80, 128, 256)and(112, 64, 256)as tuned entries, so autotuningtile_mover that table on a 2-byte dtype silently produces garbage.Reproduction — against main at
ac227c3,tile_m=112, tile_n=256, tile_k=64, in_dtype="bf16"compiles, runs, and gives a relative error of ~1.0 againsttorch.mm. After this PR the same call raisesValueErrornaming the required multiple.Bug 2 — whole-tile A-fragment read costs up to 3.3×
#974 replaced the per-
kiA-fragment copies inmma_kloopwith a singlewhole-tile
fx.copy. The two are semantically identical, but the single copy leaves the scheduler no room to interleave the reads with the MFMAs that consume them.Single-hunk A/B against main at
ac227c3, bf16, M=54560 K=8192 N=8192, sync path:The regression is confined to specific large-
tile_kshapes.128×256×64is the control; the affected cell is stable to ±0.3% over five repeats, so the 3.3× is not measurement noise.Testing
Environment: MI355X (gfx950, 256 CU), ROCm 7.2.3 / PyTorch 2.11.0 / FlyDSL 0.2.4, base commit
ac227c3(main as of #1006). This is older than CI'srocm/pytorch:rocm7.14_ubuntu24.04_py3.11_pytorch_release_2.12.0; thelinux-flydsl-mi355-8runner is the same GPU, so CI should confirm on the supported toolchain.Adds
test_preshuffle_rejects_partial_a_tile(48/80/112/144 must raise) andtest_preshuffle_accepts_whole_a_tile(64/96/128/160 must still compile). The 34 skips are the pre-existing MXFP4 fp8-A cases plus the non-8-bitasync_copyparametrization, both unchanged by this PR.Not tested on gfx942 — bug 1's check is pure Python validation with no architecture dependence; bug 2 is a scheduling change and its measurements are gfx950 only.
Breaking Changes
Configurations that previously returned silently wrong results now raise at compile time. Callers autotuning
tile_mshould catchValueErrorand skip, as they already must for the existingtile_kandlds_stagevalidation.