gfx1151 nwarps, tile sizing to curb VGPR pressure#21344
Closed
pedapudi wants to merge 6 commits into
Closed
Conversation
Author
github-actions
Bot
added
Nvidia GPU
|
Strangely, there's been no improvement on my machine. Is there any other information required? |
Author
|
@IIIIIllllIIIIIlllll thanks for looking. I'm also on Ubuntu 25.10 using ROCm 7.2.1. I'm surprised that you aren't seeing any improvement. I don't know how you applied the patch, built llama.cpp, or how you tested. I'll update the PR with my cmake flags. |
|
Here is my command: The model: https://huggingface.co/Ex0bit/Qwen3.5-122B-A10B-PRISM-LITE-GGUF My testing method: I started the model using 
|
|
I tested this PR with more models (arch linux 6.19, rocm 7.2.1, vulkan 1.4.341): Mistral-Small-4-119B-2603-UD-Q4_K_XL, Qwen3.5-122B-A10B-UD-Q4_K_XL, NVIDIA-Nemotron-3-Super-120B-A12B-UD-Q4_K_XL. (I added Vulkan, since I was also curious about the comparison ROCm/Vulkan). For ROCm, this PR performs consistently better for the 3 tested models Mistral, Qwen3.5, NVIDIA-Nemotron (with the excetion of -1% for Qwen, which also may be noise). In summary (ROCm before/after): Mistral-Small-4-119B (ROCm)
Qwen3.5-122B-A10B (ROCm)
NVIDIA-Nemotron-3-Super-120B-A12B (ROCm)
Master: (Before) This PR: (After) And just for curiosity, the comparison Vulkan/ROCm (after this PR): ROCm vs Vulkan highlights/lowlights (after PR)
|
Author
|
@tbocek Thank you for doing the cross model testing! I'm glad the PR shows uplift across models. I have some questions about your empirical numbers which look lower than what I see with llama-bench for the Qwen3.5 122B model. I re-ran llama-bench with Unsloth's UD Q4_K_XL to match your run, and I see: BEFORE $ ./bin/llama-bench --model /home/sunil/models/unsloth/qwen35-122b-ud-q4_k_xl/unsloth_Qwen3.5-122B-A10B-GGUF_UD-Q4_K_XL_Qwen3.5-122B-A10B-UD-Q4_K_XL-00001-of-00003.gguf -p 128,256,512,1024,2048,4096 -n 0 --n-gpu-layers 99 --flash-attn 1 --mmap 0 --direct-io 1 --ubatch-size 2048 --batch-size 2048 -r 5
ggml_cuda_init: found 1 ROCm devices (Total VRAM: 126976 MiB):
Device 0: AMD Radeon Graphics, gfx1151 (0x1151), VMM: no, Wave Size: 32, VRAM: 126976 MiB
| model | size | params | backend | ngl | n_ubatch | fa | mmap | dio | test | t/s |
| ------------------------------ | ---------: | ---------: | ---------- | --: | -------: | -: | ---: | --: | --------------: | -------------------: |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp128 | 181.31 ± 4.92 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp256 | 269.33 ± 3.56 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp512 | 355.28 ± 1.51 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp1024 | 418.42 ± 1.23 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp2048 | 429.09 ± 5.65 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp4096 | 405.33 ± 2.56 |
build: 7c7d6ce5c (8642)AFTER ./bin/llama-bench --model /home/sunil/models/unsloth/qwen35-122b-ud-q4_k_xl/unsloth_Qwen3.5-122B-A10B-GGUF_UD-Q4_K_XL_Qwen3.5-122B-A10B-UD-Q4_K_XL-00001-of-00003.gguf -p 128,256,512,1024,2048,4096 -n 0 --n-gpu-layers 99 --flash-attn 1 --mmap 0 --direct-io 1 --ubatch-size 2048 --batch-size 2048 -r 5
ggml_cuda_init: found 1 ROCm devices (Total VRAM: 126976 MiB):
Device 0: AMD Radeon Graphics, gfx1151 (0x1151), VMM: no, Wave Size: 32, VRAM: 126976 MiB
| model | size | params | backend | ngl | n_ubatch | fa | mmap | dio | test | t/s |
| ------------------------------ | ---------: | ---------: | ---------- | --: | -------: | -: | ---: | --: | --------------: | -------------------: |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp128 | 314.28 ± 5.58 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp256 | 411.32 ± 3.45 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp512 | 488.98 ± 2.14 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp1024 | 442.81 ± 1.63 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp2048 | 553.57 ± 5.64 |
| qwen35moe 122B.A10B Q4_K - Medium | 71.73 GiB | 122.11 B | ROCm | 99 | 2048 | 1 | 0 | 1 | pp4096 | 494.13 ± 3.09 |
build: e0e3c3fc6 (8643)(note the differing build IDs but they are not material here.) Both baseline and with this PR, the prefill t/s is higher. It's possibly worth normalizing for whatever differences exist between the setups, but it seems your findings corroborate some of the relative improvement with the PR. The falloff you are noticing at higher lengths seems to have a different slope though. |
|
@pedapudi I just realized, I had different build flags, I was using ROCWMMA_FATTN=ON. Now with Qwen3.5-122B-A10B-UD-Q4_K_XL with ROCWMMA_FATTN=OFF. Now the number are closer to yours. Summary:
before PR: and after PR: |
Author
|
@tbocek So glad that you were able to identify the discrepancy! Thanks again for testing. |
Contributor
|
@IMbackK @JohannesGaessler does this look okay? I don't have the hardware to test but a lot of people seem to have confirmed it looks good. |
Collaborator
|
I have yet to find the time for a proper review, but on a surface level im not convinced with the current state of this. For starters this is trying to solve register pressure by tuning the values for rdna3.5 using gfx1151 as a model, but other rdna3.5 gpus have different size register files. |
Author
|
Thank you, @am17an and @IMbackK.
You may be right, and it's a reasonable point to raise here, but I would like to offer a few things for you to consider before holding back on incremental improvements (within the current structure of the code, at least):
As I mentioned in my original issue, there is no static sizing that's going to work across architectures. Sidebar: One idea I'd love for llama.cpp is to have a |
Contributor
There was a problem hiding this comment.
What are your intentions with the changes to mmvq.cu? They look wrong.
Author
There was a problem hiding this comment.
I wanted to update the structure to support RDNA 3.5 more natively and not reuse preexisting configurations. I've reverted the changes for now to not distract from the other changes. Thanks!
Contributor
Looking at this table, the in my opinion correct way to do it is to use the value for discrete RDNA3 GPUs for those APUs that only have 512 kiB of registers. |
Well right now everything is tuned for RDNA2 which is even worse. Incremental improvements always make sense especially if they focus on the more popular cases (Strix Halo) over the less popular ones (Strix Point) |
Collaborator
You mean RDNA2 DGPUs presumably, the large register file RDNA3 dgpus (gfx1100 and gfx1101) also have >512 kiB of registers. |
Collaborator
|
Regarding register pressure we really only have 3 cases: gfx1100, gfx1101, gfx1151, and gfx12 with 768 32 wide vector registers Btw the table is wrong, the unit is not kiB, its the number of vector registers. A single register being 1024 bits for rdna and 2048 bits for gcn/cnda |
Author
|
Thanks for the discussion so far. I'm a little confused as to where we are. Is the suggestion to abandon this change? If so, would someone else pick up realizing the performance opportunity? If not, what are the next steps? Thank you! (PS. That table indeed looks misleading :)) |
Collaborator
I strongly disagree with this notion
If the resulting kernels are indeed below 512 registers (see GGML_HIP_EXPORT_METRICS) i think this is conceptually fine and likely to be positive for the other gfx115x gpus. Ideally someone would benchmark this fact, however. Further it makes sense to try these values on the other RDNA devices, as they would also spill and widen the filter further, i will check this on gfx1100 soon. This dosent have to be part of this pr. |
Author
|
Thank you, @IMbackK
Yes, reasonable people can disagree :) I respect your position, especially from a maintainer, that no hardware will be alienated. After all, that is the crux of this PR as well. For a later time: I think there is a structural opportunity in the implementation to support different GPU architectures with less maintenance burden, eg., organizing different config files, or an adaptive approach like my prior comment was conceptualizing using a "probe the host hardware" step.
Thank you, this is all very reasonable. AFAIK we're targeting 256 VGPR for gfx1151 (ideally slightly below to leave room for system registers). I attached the remarks from GGML_HIP_EXPORT_METRICS (which has been a useful utility in validating some additional changing outside this PR I'm trying as well!). There is still some VGPR spill (especially with IQ quantization). At least with mmq_x, if I lower it further (even for just Q8), there does not seem to have any improvement in performance and high potential for regressions. |
Contributor
Performance changes
In my testing the performance changes from this PR are very inconsistent across batch sizes and data types. It cannot be merged like this. |
Author
|
@JohannesGaessler thank you for the sweep! I understand your position that the variance is not desirable. Asking naively, is the sweep impacted by the issue described in this PR: #21282 Let me know if you have suggestions on how to reduce the variance. Thanks. I'd also appreciate your eyes on the sweep @tbocek did showing material benefit on more modern and larger models than llama 8B. |
Author
|
Perhaps corroborating your findings: this PR appears to be a benefit for MoE models, but a wildcard for dense model (eg., Qwen 3.5 9B). There might be a path forward if this change is categorically not beneficial for dense models simply by reverting to the old values if the model is dense in the mmq code path? I don't quite know why this isn't beneficial (have not had a chance to look more closely). What's your opinion? |
Contributor
|
👋 In all these benchmarks, main is First, here are some other benchmarks results with 3 MoE models at various context sizes: Gemma4 26B-A4B, Nemotron3-Super and GLM4.7-Flash. They are looking great! Then, here are some results with Qwen 3.5 9B, I've tried to use kind of the same params you were using in the llama 8B benchmarks you shared earlier @JohannesGaessler, varying the quants and the ubatch size. I'm wondering the reason you're interested in the different ubatch size, if you have some pointers? HTH |
|
Tested this on a Framework Desktop (Ryzen AI MAX+ 395, Radeon 8060S / gfx1151, 128 GB RAM, Fedora 43, ROCm 7.2.1) with: Qwen3.5-122B-A10B-REAP-20 Q6_K. I used Codex to build a patched ROCm container from the PR diff and compared it against the stock ROCm
So at least on gfx1151 + a large MoE model, this looks very real and very useful. |
|
I have added a toolbox with this PR and run the benchmark: https://kyuz0.github.io/amd-strix-halo-toolboxes/ The benefits seem to be mostly for short context, but if you switch to the 30k context tests, results do not look great, sometimes better, sometimes worse, but not by that much. |
Co-authored-by: Johannes Gäßler <johannesg@5d6.de>
Split the RDNA3.5 mmq_x_max cap by workload at runtime: - get_mmq_x_max_host returns 128 for RDNA3.5 (was 48), exposing the full AMD_WMMA_AVAILABLE device-side limit to dense GEMMs. - get_mmq_x_max_device drops the RDNA3.5=48 carve-out and follows the default 128 for AMD_WMMA_AVAILABLE, so mmq_x=128 kernel templates are instantiated. - mul_mat_q_case caps mmq_x_max back to 48 only when args.expert_bounds is non-null (the MoE path). Dense GEMMs (ggml_cuda_mul_mat_q with ids==nullptr, plus ggml_cuda_op_mul_mat_q) leave it null and pick up the 128 cap. The existing tile-selection loop walks mmq_x up to mmq_x_max and picks the largest one that minimises tile count, so dense pp1024 lands on mmq_x=128 (8 tiles, no waste) while MoE per-expert dispatches still stay at mmq_x<=48 (low fragmentation on the long tail of cold experts). Measured on Strix Halo (gfx1151), ROCm 7.2.1, clean local A/B versus this branch's prior settings (mmq_x_max=48 for both paths): | workload | prior (48) | this | delta | |--------------------------|-----------:|------:|-------:| | Dense 27B Q8 pp1024 | 292 | 422 | +44.8% | | Dense 27B Q8 pp4096 | 271 | 351 | +29.4% | | Dense 2B Q8 pp1024 | 4728 | 5172 | +9.4% | | Qwen3.5 122B MoE pp1024 | 474 | 513 | +8.2% | | Qwen3.5 122B MoE pp4096 | 512 | 534 | +4.3% | | Qwen3.5 35B MoE pp1024 | 1505 | 1540 | +2.3% | | Qwen3.5 35B MoE pp4096 | 1465 | 1508 | +3.0% | | Qwen3.5 35B MoE pp32768 | 1007 | 1032 | +2.5% | The MoE FFN path keeps the original 48 cap; dense and 122B MoE gains come from attention/projection layers picking up the 128 cap.
Adds an RDNA3.5-only config override that drops nbatch_K from 128 to 64 for the flash_attn_tile<256,256,4,8> path. Clears the 256 VGPR/wave cliff (256 -> 192) and cuts LDS/WG by ~22%, yielding +4-9% on long-context pp. Other RDNA generations are unaffected via fall-through to the general RDNA table.
Author
|
Based on @kyuz0 testing of longer depth context, I updated this PR to be more complete and also address flash attention kernel tile sizing. As alluded to in my prior comment, this does change the scope of the PR a little bit by including one more file with tile sizing changes, but this should make the PR overall more complete in addressing low hanging fruit for gfx1151 to avoid VGPR pressure. The change is as follows. Simply, drop HEAD mmq tiling, nwarps changes nbatch_K = 64 |
Contributor
|
The reason this PR is not being merged is because the changes are affecting all quantization types and are not universally beneficial. Right now this is unfortunately unavoidable because better parameterization of the MMQ kernel is needed. Piling on additional changes to the tile FA kernel (which should be their own PR) will make it even less likely for this PR to get merged. |
Author
|
@JohannesGaessler just to confirm, your concern is now primarily about quantization and no dense vs. MoE models. Is that right? Are you extrapolating the concern around quantization due to your testing of a dense model (which conflates the cause of performance variability)? |
Contributor
|
I did a sweep over quantization types and batch sizes. This PR is not consistently faster but it adds complexity. So it is a net negative for the project and should not be merged. Doubly so if you start modifying two separate kernels the changes to which should be tested and merged independently of one another. |
Author
|
Got it, thanks @JohannesGaessler! I didn't realize you did a sweep after the latest dense-aware changes to mmq from two days ago. I don't understand how that can impact any quantization for your dense 7b model because it restores the code path at head for any dense model (should be a no op), but I trust that you tested conclusively. How about this? Let's close this PR and resume conversation in the issue that was filed originally. Maintainers encouraged a PR, but my intent was to show where low hanging fruit were independent of how it is committed. Now that we have some strawman implementation which shows the headroom, we can resume the discussion in the issue. It's clear that this PR is not getting approved. Does that seem like a good next step? |
Contributor
|
No, the next step is to refactor the MMQ configuration to work more like how it's done in |
Author
Just to make sure I understand what you're saying: is the suggestion that tile size tuning is blocked on effectively rewriting mmq.cuh almost entirely? |
Contributor
|
No, as the maintainer of the CUDA device code for matrix multiplications I am telling you that until there is better infrastructure for more fine-grained tuning a PR like this will not get merged unless the impact is clearly beneficial when considering all quantization types. This is not a suggestion. |
Author
|
@JohannesGaessler It's possible that we are miscommunicating. I'm asking you what we should do with this PR. Q1: "Let's close this PR and resume conversation in the issue that was filed originally...Does that seem like a good next step?" Q2: "is the suggestion that tile size tuning is blocked?" As you may infer from Q1, I am not disagreeing with you. You made it clear this PR is not getting merged. I'm asking which of the following next steps are you suggesting?
There may be other options. Can you say what it is you are expecting? Otherwise, I'm a bit stuck on what to do with this PR. I will default to option 1 above. Thanks in advance for clarifying! |
Contributor
|
Let's close this PR then. |
Author
|
Thanks, that makes sense to me. Appreciate your engagement so far! |
Author
|
(restored the branch in case @kyuz0 is still patching the PR for toolbox benchmarks) |
Thank you! For the moment I will not keep tracking this in my daily builds and benchmarks, but the containers already published will remain available to pull. Thank you for the work you've been doing on this and trying to push performance on Strix Halo! |
chrismcmacken
added a commit
to chrismcmacken/llama.cpp
that referenced
this pull request
May 14, 2026
Consolidates per-arch MMQ tuning into a single table, per @JohannesGaessler request in ggml-org#21849 to mirror the FA orchestration pattern. No behavior change, verified on gfx1151 with test-backend-ops and llama-bench parity. Follows up PR ggml-org#21849, to allow tuning MMQ params per arch and perf PR with the gfx1151 tuning from ggml-org#21344 will sit on top of this orchestration, hopefully Co-authored-by: Antoine Viallon <antoine@lesviallon.fr> Co-authored-by: Sunil Pedapudi <424940+pedapudi@users.noreply.github.com>
chrismcmacken
added a commit
to chrismcmacken/llama.cpp
that referenced
this pull request
May 14, 2026
Consolidates per-arch MMQ tuning into a single table, per @JohannesGaessler request in ggml-org#21849 to mirror the FA orchestration pattern. No behavior change, verified on gfx1151 with test-backend-ops and llama-bench parity. Follows up PR ggml-org#21849, to allow tuning MMQ params per arch and perf PR with the gfx1151 tuning from ggml-org#21344 will sit on top of this orchestration, hopefully Co-authored-by: Antoine Viallon <antoine@lesviallon.fr> Co-authored-by: Sunil Pedapudi <424940+pedapudi@users.noreply.github.com>
justinappler
added a commit
to justinappler/llama.cpp-strix-halo
that referenced
this pull request
May 15, 2026
…nary layout Fresh re-port of the six-edit RDNA3.5 MMQ tuning patch (Finding ggml-org#5) after the 2026-05-14 upstream rebase. The original 294b335 form edited an if-chain in mmq.cuh; upstream PR ggml-org#22051 + follow-ups refactored those helpers into nested ternaries, so the patch is rewritten to fit: - get_mmq_x_max_host: RDNA3_5 -> 48 (ternary prepended) - get_mmq_x_max_device: AMD_WMMA branch split to RDNA3_5=48 / else=128 - get_mmq_y_host: RDNA3_5 joins the RDNA1 -> 64 case - get_mmq_y_device: #if RDNA1 || RDNA3_5 -> 64 - mmq_get_nwarps_host: RDNA3_5 -> 4 (ternary prepended) - mmq_get_nwarps_device: #if RDNA3_5 -> 4 inside AMD_MFMA||AMD_WMMA Net +18/-6 lines vs upstream. Logical patch identical to 294b335; expected direction-of-win unchanged from pre-rebase bench (see doc). Re-bench against the Qwen 3.6 matrix in strix-halo/qwen3.6-baseline.md is the gating step before this is declared kept. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
justinappler
added a commit
to justinappler/llama.cpp-strix-halo
that referenced
this pull request
May 23, 2026
…nary layout Fresh re-port of the six-edit RDNA3.5 MMQ tuning patch (Finding ggml-org#5) after the 2026-05-14 upstream rebase. The original 294b335 form edited an if-chain in mmq.cuh; upstream PR ggml-org#22051 + follow-ups refactored those helpers into nested ternaries, so the patch is rewritten to fit: - get_mmq_x_max_host: RDNA3_5 -> 48 (ternary prepended) - get_mmq_x_max_device: AMD_WMMA branch split to RDNA3_5=48 / else=128 - get_mmq_y_host: RDNA3_5 joins the RDNA1 -> 64 case - get_mmq_y_device: #if RDNA1 || RDNA3_5 -> 64 - mmq_get_nwarps_host: RDNA3_5 -> 4 (ternary prepended) - mmq_get_nwarps_device: #if RDNA3_5 -> 4 inside AMD_MFMA||AMD_WMMA Net +18/-6 lines vs upstream. Logical patch identical to 294b335; expected direction-of-win unchanged from pre-rebase bench (see doc). Re-bench against the Qwen 3.6 matrix in strix-halo/qwen3.6-baseline.md is the gating step before this is declared kept. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
4 tasks
justinappler
added a commit
to justinappler/llama.cpp-strix-halo
that referenced
this pull request
Jun 27, 2026
…nary layout Fresh re-port of the six-edit RDNA3.5 MMQ tuning patch (Finding ggml-org#5) after the 2026-05-14 upstream rebase. The original 294b335 form edited an if-chain in mmq.cuh; upstream PR ggml-org#22051 + follow-ups refactored those helpers into nested ternaries, so the patch is rewritten to fit: - get_mmq_x_max_host: RDNA3_5 -> 48 (ternary prepended) - get_mmq_x_max_device: AMD_WMMA branch split to RDNA3_5=48 / else=128 - get_mmq_y_host: RDNA3_5 joins the RDNA1 -> 64 case - get_mmq_y_device: #if RDNA1 || RDNA3_5 -> 64 - mmq_get_nwarps_host: RDNA3_5 -> 4 (ternary prepended) - mmq_get_nwarps_device: #if RDNA3_5 -> 4 inside AMD_MFMA||AMD_WMMA Net +18/-6 lines vs upstream. Logical patch identical to 294b335; expected direction-of-win unchanged from pre-rebase bench (see doc). Re-bench against the Qwen 3.6 matrix in strix-halo/qwen3.6-baseline.md is the gating step before this is declared kept. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
11 participants
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.
Follow up on issue #21284
a. MMQ:
mmq_x_max=48,mmq_y=64,nwarps=4for RDNA3_5 to balance VGPR usage and occupancyb. Note: I took the opportunity for a minor refactor replacing nested ternary operators to improve readability and reduce opportunity for errors (especially after I made a mistake while piling on the ternary operations).
mmvq_parameter_table_idinstead of falling back to RDNA2a. Results in nwraps calculation falling to 1.
1 is more important than 2, but 2 is still helpful on the mmvq paths. And it sets up for future per-quant tuning.
Benchmarks
Built with cmake flags
Before (build 7c7d6ce / 8642)
After (build 955df3551 / 8643)
Speedup
Requirements