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
|
@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. |
|
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. |
This does not seem right. If the HIP allocates CPU buffers, they should end up in separate graph splits. If they don't then there is some problem in the HIP backend and should be fixed there. |
|
@ggerganov these are not technically CPU buffers, they're GPU_Host buffers that just happen to share the same memory as the CPU buffers (due to memory pin). |
|
This is a specific to integrated systems where you can remap the same memory location to be either CPU or GPU. |
|
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
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) |
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