ggml-cuda: avoid direct ROCm_Host compute on HIP integrated GPUs - #25863
ggml-cuda: avoid direct ROCm_Host compute on HIP integrated GPUs#25863Victor-Loos wants to merge 2 commits into
Conversation
github-actions
Bot
added
ggml
|
@liminfei-amd could you please take a look? |
|
Thanks for the ping. I tested this on gfx1151 / ROCm 7.2.1. Before #24233, the host-buffer capability and factory were already enabled; the newly reachable behavior was The current patch fixes this conservatively, but disabling the factory reduced pp512 from 5857 to 1502 t/s. I suggest a narrower fix: This keeps pinned staging/output buffers but prevents direct HIP APU computation on |
8a02d7f to
fdc1260
Compare
|
Thanks @liminfei-amd for the testing and for clarifying the narrower fix. I tested your suggested variant with my setup, and it still fixes the issue. I've updated the PR accordingly. As for performance, on my setup I didn't see any meaningful difference. Here are my llama-bench results with a Qwen3.6-35B-A3B model:
Could you retest the updated version on your side and confirm whether it preserves the pp512 result you mentioned? If there's still a difference, could you share the exact command and model you used so I can try to reproduce it? |
|
Hello I ran into issues on my strix halo box and this PR fixes it. Step 3.7 Flash IQ4_XS will spam I have now tested this pull request on my box and its solved. |
|
Thanks @Victor-Loos Retested Two small changes are still needed before merge:
#else
GGML_UNUSED(device);
Apart from these two items, the revised implementation looks good to me. |
Victor-Loos
changed the title
|
Thanks, @liminfei-amd. I’ve addressed both remaining points. Could you please take another look when you have time? |
|
BTW @liminfei-amd maybe you know the reason behind #15018 ? I've ran into this today as well and I've tried everything, but can't get over the weird 64GB allocation soft limit on ROCm with |
|
@Victor-Loos I rechecked the latest head. The requested code and PR description changes look good. The exact-parent/head ABBA result remains applicable: pp512 across I still have not independently reproduced the Pi/OpenCode corruption itself, but the requested changes validate cleanly on my side. |
@pwilkin Thanks for the ping. I'm currently trying to reproduce the issue and find a solution. |
|
Hi! I also ran into this regression. Host: Strix Halo 128GB Behavior before #24233Models call properly structured tool calls. Malformed tool calls are rare. Behavior since #24233Models output intermittent, but persistent, malformed tool calls, which are hard to diagnose. Most often, model would output malformed tool calls after cold boot, and the same behavior would persist after repeated invocations with fresh context. After repeating tests with fresh context and slightly modified prompt (e.g. "please test the Non-exhaustive examples of observed malformed tool callsModels used
Build from this PR outputs consistently structured good tool calls, so far (tested 6 invocations per model, fresh context each time) |
|
@IMbackK This PR hardens the HIP integrated-GPU support added in #24233 by keeping the correct iGPU classification and pinned ROCm_Host allocation, while preventing unsafe direct computation on ROCm_Host tensors. I independently reproduced #25992 on gfx1151 with a Qwen3.6-35B model using mmap, -np 4, and --kv-unified. Without this fix, the server produced 2 cross-request replays in 96 complete responses. With the latest PR head, all 96 responses completed without replay, request errors, or nonce misses. Please review and merge when possible. Thanks! |
|
Confirmed fixed on independent gfx1151 hardware (AMD Strix Halo / Ryzen AI Max APU), ROCm 7.14. I'm the reporter of #25992 (the Model fully offloaded to the gfx1151 iGPU (confirmed via Result — with this PR: 12 rounds × 4 concurrent requests, each carrying a unique nonce behind a ~24k-char filler prompt to exercise the host/pinned-buffer path under load → 48/48 responses returned their own nonce, 0 cross-slot leakage. On the affected build the same setup returned other requests' responses verbatim across slots (#25992). Prompt input is also no longer corrupted here — system/user content is honored correctly again (it was ignored/garbled before). The change is surgical and correct for this case: it disables only the direct ROCm host-buffer compute path on HIP integrated GPUs while keeping integrated-GPU detection and pinned buffers intact. LGTM from a gfx1151 user — would love to see this merged so we can drop our backend pin. Thanks @Victor-Loos. |
|
@IMbackK This one's pretty critical, so unless there's something wrong I'd be for merging it ASAP. |
|
I've tested out this fix with gfx1150 (Radeon 890M), ROCm 7.2.4, and seems to work (with the confirmation that the same tests failed on the current head). |
|
This looks like it would be a potential data-race/stale pointer. Mind trying #26167 to see if anything pops up?
If I understand the current code correctly, prop.integrated serves only this aspect (i.e. to facilitate use on host-buffers). So we may look to revert #24233 and get this fixed for both CUDA and ROCm. For CUDA, we have limited interest in host-buffer-use atm as the saved copies of inputs/outputs are offset by the cost of full cache-coherency between iGPU and CPU caches on our current UMA devices. Quoting from the CUDA docs on memory coherency for iGPUs:
I cannot comment on perf for AMD/Intel's iGPUs, but would assume memory coherency is cheap for Apple as they don't even expose the concept of an "exclusively owned pointer) (cf. MLX). |
|
@ORippler I'll try to run something with the sanitizer and see if I can find anything. |
|
@ORippler I built the #26167 sanitizer and ran the same reproduction with GGML_SCHED_SANITIZE=1. Without this fix, the server aborts with: The host rewrites prompt tokens in the ROCm_Host-backed inp_tokens tensor while an asynchronous HIP operation may still be reading from it. There is no synchronization establishing that the HIP read completed before the host write. With this PR applied, I repeated the same reproduction with the sanitizer enabled, and the race was no longer reported. |
|
With #26225 this workaround is no longer needed. |
|
The DGX Spark and the Macs are also integrated systems and don't need this patch. |
|
Originally this workaround was done for NVIDIA Jetson Orin and I'm not really sure they "don't need this patch", as the direct host compute was only reenabled for HIP-backed integrated GPUs so far, the other CUDA systems stay on #16308 - so you'd have to remove the gate and test with the sanitizer (#26167) on Spark to see if it indeed doesn't surface. |
Yeah CUDA UMA systems should be affected by this as well, see the original thor issue that facilitated disablement.
DGX Spark doesn't behave like a UMA system atm, and we expect reading from host buffers to be perf-neutral for the aforementioned cache issues: #25863 (comment). Does the metal backend expose/use pinned host memory? I'd favor a solution where we add a ring-buffer of input/outputs in the scheduler rather than placing more syncs |
|
I took you PR and applied your commits to latest llama.cpp (https://github.com/jj123451/llama.cpp). I built it locally on evo x2 (Strix Halo) with rocm 7.14 and tested it in parallel on 14 prompts using gemma4. I did the same test with latest original llama.cpp built the same way.
thx for your effort |
No, it only wraps host buffers in Metal buffers, but does not declare a host buffer type. I guess that's the reason to not observe the issue with Metal. |
IMbackK
left a comment
IMbackK
left a comment
There was a problem hiding this comment.
I dont have the hardware to test that this solves the problem or to reproduce the original issue, logically the original path should be fine to.
I have to trust in others here that this is the best mitigation.
|
@ggml-org/ggml-cuda a second approval would be welcome. |
|
Confirmed on Strix Halo (gfx1151, Radeon 8060S), ROCm 7.14, Windows. |
@IMbackK are there other use-cases for prop.integrated? If not, this "fix" reverts the only use-case that would want to be enabled. In that case, we should simply revert #24233 in my eyes, to not fragment CUDA/HIP parts unnecessarily |
|
@pwilkin I dont know what is actually wrong with the host buffer usage as is and I dont have a uma system to actually validate any change i would make so i dont actually have any recourse other than to just disable the thing and hope someone else fixes it. |
@pwilkin happy to pair/assist to ensure it runs smoothly on DGX/RTX Spark also. Note this will involve work on the scheduler, which we are in the process of testing more robustly (@aendk is working on this afaik) |
|
There's been a series of work related to Jetson and HIP devices that work around bugs with integrated GPU:
The core problem appears to be the scheduler not protecting user input when that input is chunked. With Claude's help I was able to build a fix and test at master...alexpooley:llama.cpp:fix/ggml-copy-caller-writable-inputs If you have a HIP device you can run the test with and without the fix in place. If you have a Jetson device you can undo the change in https://github.com/ggml-org/llama.cpp/pull/16308/changes and try the fix instead. Furthermore the perplexity score saw a significant change: Model is Qwen3-4B-Instruct-2507-Q8_0, corpus is the wikitext-2 raw test set, ROCm/HIP on gfx1151.
It would be good if someone could take a look at the linked fix and confirm. @liminfei-amd maybe? AI usage disclosure: YES - I used AI assistance to identify the bug, build the fix and regression test. I tested and reviewed the final change myself. |
…ild attribution, config caveats) - measurement labels: the code-decode baseline is now the like-for-like 24.4 t/s plain run (23.5 was a different day, config and generation length); @64k -> @78k (77,669 tokens); pp @131k split into a delta-rate row and a 156K-prompt-average row; the "~502" tilde dropped (it is a direct llama-bench measurement); repeat artifact +34% not ~50%, cold start up to -55%. - build attribution: stock column, depth-curve baseline, ngram-mod solo gain, IQ4 decode cost (8-11% stock vs ~7% patched), multi-slot table and the IQ3 MTP combo (35.3/25.1 in the full build) now name the build they were measured on; the graph-reuse commit's engram row prefetch and IQ4_NL get_rows path are documented. - config caveats: config A drops LLAMA_QSA_GATHER=1 (the default threshold already applies; the variable is an n_kv threshold, not a switch), config C gains LLAMA_QSA_GATHER=0 and the ggml-org#25992 multi-slot correctness warning, --tensor-read-lazy documented as already-auto for this tensor, GGML_HIP_GDN_CHUNK documented and marked inactive in the numbers. - ggml-org#25992 patch described by what it does (disables ROCm host-buffer compute on integrated GPUs; correctness workaround based on the open PR ggml-org#25863), the zero-copy claim removed, RAM-mode numbers flagged as being re-verified. - claims narrowed to what was measured: PPL 0.03% applies to the gather A/B in the same build, NMSE 4.4e-14 is an ad-hoc measurement not a checked-in test, temperature-0 losslessness is an argument not a measurement, resident engram footprint is an estimate, 256K+MTP was never run, deadlock reproduced at 143360/163840 with 32K-98K untested. - outside references: LaurentZuijdwijk credited by name with branch link, dzannotti HF link, related-work heading separates prior art from parallel work, license link points at LICENSE and names the MAU/MaaS clauses, BENCHMARKS base commit labeled as the PR state at measurement time, Vulkan paragraph marked as an unverified third-party report, raw-data channel names the fork's issue tracker (now enabled) alongside the HF sidecar discussions. - units: GiB throughout for the engram table. - conversion/qwen4exp.py: dead vLLM comment replaced (the MTP block is exported as a sidecar). - QSA gather described at its current state: multi-sequence ubatches fall back to the masked path by default, LLAMA_QSA_GATHER_MS=1 opts them in and LLAMA_QSA_GATHER_TRACE=1 logs every gather graph build. The NMSE figure now points at tests/test-qsa-gather-ms.cpp instead of an ad-hoc measurement. Config C loses its LLAMA_QSA_GATHER=0 line - the default gate already keeps multi-slot decode off the gather - while the ggml-org#25992 correctness warning for multi-slot serving stays. - container reproducibility: the measured builds applied a second workaround that the published recipe did not ship, a per-buffer mmap loader patch (drops the blanket use_mmap bail-out from the async upload path, records per shard buffer whether it is mmap-backed, and stops the loader from prefetching every byte of every shard). It is added as docs/strix-halo/llama-cpp-qwen38-per-buffer-mmap.patch and applied by Dockerfile.rocm-7.14 under the same "apply while it still fits" guard as the ggml-org#25992 patch, so a container built from the recipe reproduces the measured configuration. The RAM-mode re-verification note is resolved accordingly. - branch state: the commit table names the commits as they are now called, and the intro says the branch is rebased onto master past the ggml-org#27742 merge instead of pinned to a pre-merge state.
…ild attribution, config caveats) - measurement labels: the code-decode baseline is now the like-for-like 24.4 t/s plain run (23.5 was a different day, config and generation length); @64k -> @78k (77,669 tokens); pp @131k split into a delta-rate row and a 156K-prompt-average row; the "~502" tilde dropped (it is a direct llama-bench measurement); repeat artifact +34% not ~50%, cold start up to -55%. - build attribution: stock column, depth-curve baseline, ngram-mod solo gain, IQ4 decode cost (8-11% stock vs ~7% patched), multi-slot table and the IQ3 MTP combo (35.3/25.1 in the full build) now name the build they were measured on; the graph-reuse commit's engram row prefetch and IQ4_NL get_rows path are documented. - config caveats: config A drops LLAMA_QSA_GATHER=1 (the default threshold already applies; the variable is an n_kv threshold, not a switch), config C gains LLAMA_QSA_GATHER=0 and the ggml-org#25992 multi-slot correctness warning, --tensor-read-lazy documented as already-auto for this tensor, GGML_HIP_GDN_CHUNK documented and marked inactive in the numbers. - ggml-org#25992 patch described by what it does (disables ROCm host-buffer compute on integrated GPUs; correctness workaround based on the open PR ggml-org#25863), the zero-copy claim removed, RAM-mode numbers flagged as being re-verified. - claims narrowed to what was measured: PPL 0.03% applies to the gather A/B in the same build, NMSE 4.4e-14 is an ad-hoc measurement not a checked-in test, temperature-0 losslessness is an argument not a measurement, resident engram footprint is an estimate, 256K+MTP was never run, deadlock reproduced at 143360/163840 with 32K-98K untested. - outside references: LaurentZuijdwijk credited by name with branch link, dzannotti HF link, related-work heading separates prior art from parallel work, license link points at LICENSE and names the MAU/MaaS clauses, BENCHMARKS base commit labeled as the PR state at measurement time, Vulkan paragraph marked as an unverified third-party report, raw-data channel names the fork's issue tracker (now enabled) alongside the HF sidecar discussions. - units: GiB throughout for the engram table. - conversion/qwen4exp.py: dead vLLM comment replaced (the MTP block is exported as a sidecar). - QSA gather described at its current state: multi-sequence ubatches fall back to the masked path by default, LLAMA_QSA_GATHER_MS=1 opts them in and LLAMA_QSA_GATHER_TRACE=1 logs every gather graph build. The NMSE figure now points at tests/test-qsa-gather-ms.cpp instead of an ad-hoc measurement. Config C loses its LLAMA_QSA_GATHER=0 line - the default gate already keeps multi-slot decode off the gather - while the ggml-org#25992 correctness warning for multi-slot serving stays. - container reproducibility: the measured builds applied a second workaround that the published recipe did not ship, a per-buffer mmap loader patch (drops the blanket use_mmap bail-out from the async upload path, records per shard buffer whether it is mmap-backed, and stops the loader from prefetching every byte of every shard). It is added as docs/strix-halo/llama-cpp-qwen38-per-buffer-mmap.patch and applied by Dockerfile.rocm-7.14 under the same "apply while it still fits" guard as the ggml-org#25992 patch, so a container built from the recipe reproduces the measured configuration. The RAM-mode re-verification note is resolved accordingly. - branch state: the commit table names the commits as they are now called, and the intro says the branch is rebased onto master past the ggml-org#27742 merge instead of pinned to a pre-merge state.
…ild attribution, config caveats) - measurement labels: the code-decode baseline is now the like-for-like 24.4 t/s plain run (23.5 was a different day, config and generation length); @64k -> @78k (77,669 tokens); pp @131k split into a delta-rate row and a 156K-prompt-average row; the "~502" tilde dropped (it is a direct llama-bench measurement); repeat artifact +34% not ~50%, cold start up to -55%. - build attribution: stock column, depth-curve baseline, ngram-mod solo gain, IQ4 decode cost (8-11% stock vs ~7% patched), multi-slot table and the IQ3 MTP combo (35.3/25.1 in the full build) now name the build they were measured on; the graph-reuse commit's engram row prefetch and IQ4_NL get_rows path are documented. - config caveats: config A drops LLAMA_QSA_GATHER=1 (the default threshold already applies; the variable is an n_kv threshold, not a switch), config C gains LLAMA_QSA_GATHER=0 and the ggml-org#25992 multi-slot correctness warning, --tensor-read-lazy documented as already-auto for this tensor, GGML_HIP_GDN_CHUNK documented and marked inactive in the numbers. - ggml-org#25992 patch described by what it does (disables ROCm host-buffer compute on integrated GPUs; correctness workaround based on the open PR ggml-org#25863), the zero-copy claim removed, RAM-mode numbers flagged as being re-verified. - claims narrowed to what was measured: PPL 0.03% applies to the gather A/B in the same build, NMSE 4.4e-14 is an ad-hoc measurement not a checked-in test, temperature-0 losslessness is an argument not a measurement, resident engram footprint is an estimate, 256K+MTP was never run, deadlock reproduced at 143360/163840 with 32K-98K untested. - outside references: LaurentZuijdwijk credited by name with branch link, dzannotti HF link, related-work heading separates prior art from parallel work, license link points at LICENSE and names the MAU/MaaS clauses, BENCHMARKS base commit labeled as the PR state at measurement time, Vulkan paragraph marked as an unverified third-party report, raw-data channel names the fork's issue tracker (now enabled) alongside the HF sidecar discussions. - units: GiB throughout for the engram table. - conversion/qwen4exp.py: dead vLLM comment replaced (the MTP block is exported as a sidecar). - QSA gather described at its current state: multi-sequence ubatches fall back to the masked path by default, LLAMA_QSA_GATHER_MS=1 opts them in and LLAMA_QSA_GATHER_TRACE=1 logs every gather graph build. The NMSE figure now points at tests/test-qsa-gather-ms.cpp instead of an ad-hoc measurement. Config C loses its LLAMA_QSA_GATHER=0 line - the default gate already keeps multi-slot decode off the gather - while the ggml-org#25992 correctness warning for multi-slot serving stays. - container reproducibility: the measured builds applied a second workaround that the published recipe did not ship, a per-buffer mmap loader patch (drops the blanket use_mmap bail-out from the async upload path, records per shard buffer whether it is mmap-backed, and stops the loader from prefetching every byte of every shard). It is added as docs/strix-halo/llama-cpp-qwen38-per-buffer-mmap.patch and applied by Dockerfile.rocm-7.14 under the same "apply while it still fits" guard as the ggml-org#25992 patch, so a container built from the recipe reproduces the measured configuration. The RAM-mode re-verification note is resolved accordingly. - branch state: the commit table names the commits as they are now called, and the intro says the branch is rebased onto master past the ggml-org#27742 merge instead of pinned to a pre-merge state.
13 participants
Overview
Fixes a regression after #24233, which restored HIP integrated GPU detection. On my AMD APU, this also made the ROCm host-buffer path available for HIP integrated GPUs. With that path enabled, prompt input appears corrupted: chat templates and system prompts can be ignored, and generated output becomes garbage.
This change keeps integrated GPU detection/reporting intact, but disables direct ROCm_Host compute on HIP integrated GPUs. The fix does not disable host-buffer exposure entirely, pinned buffers remain available to preserve functionality, while only the problematic direct compute path is blocked.
Additional information
Tested locally on gfx1151 / RDNA3.5 APU with ROCm 7.14. The corrupted input/output issue is fixed with this patch.
The issue appeared in external harnesses like Pi and OpenCode. It can also be reproduced in the built-in web UI with a sufficiently large combined first input. For example, gemma-4-26B-A4B with 4676 tokens of context or more outputs
<unused49>indefinitely. Other models can be way more resilient and their output starts by saying 'Based on the text provided, which appears to be a corrupted or "hallucinated" version of a Wikipedia article'.Requirements