HIP: fix MMQ tile-barrier race and tune tile width for RDNA3.5 - #75
Merged
Conversation
load_tiles() overwrites tile_x while other warps of the same block can still be inside the previous iteration's vec_dot(), which reads it. Nothing separated those two accesses: the __syncthreads() calls further down all sit between the y tile store and vec_dot, on the wrong side of the hazard, and the barrier at the top of the loop was mmq_hip_tile_barrier<mmq_x>(), which is elided for mmq_x <= 32. With nwarps=4 on RDNA3.5 that left mmq_x of 16 and 32 running the whole K loop unprotected. Over 33 shapes x 7 batch sizes x 6 tile widths, 115 of 1568 configurations returned wrong results, non-deterministically: the same q8_0 8192x2048 case repeated three times gave max abs errors of 9.49, 1.37 and 4.44 against a correct value of 3e-05. It also made test-backend-ops -o MUL_MAT_ID fail intermittently on q4_0. MoE is worst affected because every MoE launch on this architecture runs at mmq_x=16. Use __syncthreads() directly there. Afterwards all 1568 configurations agree and repeated runs are bit-identical. The remaining three call sites are scheduling hints only; making them unconditional as well changes no output, so they stay elided. Cost is 1.3% at mmq_x=16 and 0.8% at mmq_x=32, nothing elsewhere.
Sweeping the shapes 20 models execute splits cleanly between the two paths: dense: 88 shapes, 11346 ms -> 11348 ms at the best width per shape (1.000x) MoE: 9 shapes, 1784 ms -> 1202 ms (1.485x) The occupancy heuristic is already optimal on every dense shape, so it stands except for one cliff: mmq_x=64 with q8_0 is 6-7x slower than the neighbouring widths, while the other types stay within 1.05x and keep it. The heuristic lands there whenever ncols_to_tile falls in (48, 64], which for a dense GEMM means a 64-token batch, and is why pp64 came out slower than pp32. MoE wants a far narrower tile: 32 is best on 8 of the 9 MoE shapes, worth 1.23x-1.71x each. The compacted per-expert grid already supplies the parallelism that column tiling supplies for a dense GEMM, so a wider tile only adds padding. q8_0 is the exception again, its narrow tiles being slow. The tuned width is checked against the same granularity, mmq_x_max and shared memory limits as the heuristic, and falls back to it otherwise. Guarded by GGML_CUDA_CC_IS_RDNA3_5. Measured end to end at pp2048 -ub 2048, builds alternated to cancel drift: Qwen3.6-35B-A3B 1.138x Qwen3.5-35B-A3B 1.130x gemma-4-26B-A4B 1.094x GLM-4.7-Flash 1.034x and from the dense cliff, pp64 at -ub 512: Qwen3.5/3.6-35B-A3B 1.74x GLM-4.7-Flash 1.21-1.31x The 16 dense models in the set run identical code through this path and act as a negative control: median 0.992x, range 0.963-1.049x, the noise floor of the machine.
roberteg16
force-pushed
the
rogarcia.mmq-rdna35-tile-tuning
branch
from
July 29, 2026 09:55
5d230db to
4ea5f29
Compare
3 tasks
roberteg16
marked this pull request as ready for review
July 29, 2026 13:41
roberteg16
requested review from
Annieren,
Copilot,
jimw567,
liangliangchang and
mgehre-amd
July 29, 2026 13:41
There was a problem hiding this comment.
🟢 Ready to approve
The synchronization fix addresses a concrete race, and the RDNA3.5 tuning is scoped and guarded by existing granularity/shared-memory eligibility checks.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes a correctness race in the HIP MMQ kernel’s K-loop (shared tile_x reuse across iterations) and adjusts RDNA3.5-specific tile-width selection to avoid known slow configurations and improve MoE throughput.
Changes:
- Add a block-wide synchronization at the top of the MMQ K-loop before
load_tiles()to preventtile_xbeing overwritten while other warps are still executing the prior iteration’svec_dot(). - Introduce an RDNA3.5 HIP-only tuned override for
mmq_x(notably avoiding themmq_x=64Q8_0 cliff, and preferring narrower tiles for MoE), applied only when the tuned width is valid for the shape (granularity + shared-memory constraints).
File summaries
| File | Description |
|---|---|
| ggml/src/ggml-cuda/mmq.cuh | Adds a missing synchronization in the MMQ K-loop and applies RDNA3.5 HIP-only mmq_x tuning with validity guards. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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 commits: a correctness fix that stands alone, and a tile-width change measured on top of it.
1. Missing tile barrier at the top of the MMQ K loop.
load_tiles()overwritestile_xwhile other warps of the same block can still be inside the previous iteration'svec_dot(). The guard there wasmmq_hip_tile_barrier<mmq_x>(), which is elided formmq_x <= 32, and the__syncthreads()calls further down all sit between the y tile store andvec_dot, on the wrong side of the hazard.With
nwarps=4on RDNA3.5 that leftmmq_xof 16 and 32 unprotected for the whole K loop. 115 of 1568 measured configurations returned wrong results, non-deterministically — the sameq8_0 8192x2048case repeated three times gave max abs errors of 9.49, 1.37 and 4.44 against a correct value of 3e-05 — andtest-backend-ops -o MUL_MAT_IDfailed intermittently on q4_0. MoE is worst hit, since every MoE launch on this architecture runs atmmq_x=16. After the fix all 1568 configurations agree and repeat runs are bit-identical. Cost is 1.3% atmmq_x=16, nothing above 32.2. Tuned tile width for RDNA3.5. Sweeping the 99 shapes that 20 models dispatch to
mul_mat_q, best width per shape vs. what the heuristic picks:Dense is already optimal and is left alone, except for
mmq_x=64with q8_0 — a 6-7x cliff the heuristic hits wheneverncols_to_tilefalls in (48, 64], i.e. a 64-token batch. MoE gets a 32-wide tile instead: the compacted per-expert grid already supplies the parallelism that column tiling gives a dense GEMM.Measured end to end
Alternated builds,
-ub 512unless noted. Whether the change fires depends onncols_to_tile = min(2*ceil(M*top_k/n_experts), M), so it moves with both the batch size and the routing shape.-ub 2048pp128 is neutral by construction:
ncols_to_tilelands on 8-16 there, which the heuristic already picks, so the rule is a no-op and only the barrier's 1.3% remains — below the noise floor.The
-ub 2048row comes from a 20-model run whose 16 dense models go through identical code on this path and act as a negative control: median 0.992x, range 0.963-1.049x.Test plan
test-backend-ops test -o MUL_MAT -b ROCm0and-o MUL_MAT_ID -b ROCm0— pass, and MUL_MAT_ID no longer fails intermittentlyllama-benchpp64/128/512/2048 at the default-ub, and pp2048-ub 2048across 20 models (0.5B-35B, dense and MoE), builds alternatedGGML_CUDA_MMQ_DEBUG=1before each A/B, so a build that does not actually apply the rule cannot be mistaken for a null result__syncthreads()should be restricted to RDNA3.5: it is active on every backend and the same hazard applies there, but it has only been validated on gfx1151Notes
gfx11, notmaster.