ggml-cuda: avoid direct ROCm_Host compute on HIP integrated GPUs (port of ggml-org#25863) - #158
ggml-cuda: avoid direct ROCm_Host compute on HIP integrated GPUs (port of ggml-org#25863)#158danielhanchen wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Port of ggml-org#25863 at ce82541, unmodified except for dropping that PR's file mode change. c7d8722 (ggml-org#24233) restored prop.integrated on HIP builds. CUDA keeps it off, with the comment 'Temporarily disabled due to issues with corrupted output'. With it on, the scheduler may place a compute input in pinned host memory, where an H2D input write can race a graph that is still running. Two reporters bisected to that commit independently, from different symptoms: ggml-org#25992 gets another slot's response verbatim under -np 4 --kv-unified, and ggml-org#27506 sees perplexity go from 7.72 to 3024 on Llama-3.2-3B. lemonade-sdk/lemonade#3160 is the same shape on our own Qwen3.8-27B GGUF. Scope, checked rather than assumed: info.devices[id].integrated is set from prop.integrated only under GGML_USE_HIP, and is hard false on CUDA and MUSA (ggml-cuda.cu:305-309), so the supports_buft change is dead code off HIP. Discrete AMD is unaffected because prop.integrated is false there. One hunk is broader: get_host_buffer_type now returns nullptr when GGML_CUDA_NO_PINNED is set, on every backend. That is a consistency fix, since props->host_buffer already reported false while the buffer type was still handed out. The only caller in the tree, ggml-backend-meta.cpp:379, handles nullptr. Pinned in scripts/unsloth/pr-set.json so the nightlies carry it; the pin comes out when a base tag contains the upstream merge.
bfe2398 to
abfc45b
Compare
…161) * unsloth: repin ggml-org#25731 to the commit that merges onto b10705 edee0e1 stopped applying to the current base tag, which failed the pin preflight. Because the preflight stops at the first conflict, it also meant the two AMD fixes pinned by #160 were never dry-run merged at all. 44eb88e is the head after the upstream conflicts were resolved; the PR is MERGEABLE upstream again as of 2026-08-31T08:06Z. * unsloth: repoint the AMD pins at commits based on an upstream ancestor #157 and #158 were branched from fork master, which is right for a small PR diff and wrong for a pin. Fork master has diverged from upstream (it deletes a dozen upstream workflow files), so merging those commits onto the base tag dragged the whole divergence in and conflicted modify/delete on twelve .github/workflows files. That is what exited the preflight with no message after 'ok #137'. The pins that work are branched from an upstream commit that is an ancestor of both master and the base tag, so they carry only their own delta. Both branches are now rebased onto 11cd988 on that pattern, and verified locally to merge onto b10705 for a net +35/-3 across 4 files, which is the two fixes and nothing else. --------- Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>
…les it (#162) ggml tests getenv(...) != nullptr, so =0 enables managed allocation. With #157 pinned this is no longer a correctness bug, but it still costs about 15% throughput and can lower the ceiling: managed draws host RAM instead of the device carve-out rather than adding to it, so a model that loads without the variable can be OOM-killed with it. Verified locally to co-merge onto b10705 with #157 and #158, net +79/-5 across 4 files. Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>
Measured on gfx1151: mainline is broken, this fork's prebuilt is notThe Verification section says this is "not yet measured on our runner". It is now. Ryzen AI Max+ PRO 395 / Radeon 8060S (gfx1151), 128 GB, Windows 11, driver 32.0.31041.1004, 96 GiB graphics carve-out, AC power irrelevant (correctness only). Method is the one this PR describes: corruption tracks Result
~1500x, and it vanishes by changing Three independent distributions of the same defect, including a third-party one (Lemonade) that ships its own ROCm llama.cpp. Note Lemonade defaults to The fix is in this fork's prebuilts somewhere between b10472 (2026-08-18) and b10798 (2026-09-04), even though #147 is closed and this PR is still open. GPU use was verified, because a silent CPU fallback fooled me onceMy first mainline attempt ran on CPU and returned a clean 11.7013 for both cells. The ggml-org Both cells above show To make the mainline package run I staged the ROCm 7.14.0 runtime beside it ( The current prebuilt is clean beyond the 1B caseSame corrupting regime,
The last two are the models from ggml-org#25992 and lemonade-sdk/lemonade#3160 respectively. Cross-slot check for the ggml-org#25992 symptom, which perplexity cannot see: Also confirms #149 on the same hardware
So Worth noting the failure mode at a large carve-out is not corruption but host starvation: managed pages come from the 31.78 GiB of visible RAM rather than the 96 GiB carve-out, so a 42.90 GiB model cannot fit and the machine goes to its knees. Forcing What this does not establish
|
1 participant
Overview
Carries ggml-org#25863 at
ce82541ainto the fork, unmodified except for dropping that PR's file mode change (it flipsggml-cuda.cuto100755; kept at100644here).25 insertions, 2 deletions, one file. Companion to #147, which pins the same upstream commit into the nightlies. This one puts the code in the fork so it is reviewable here and survives as a branch of record.
The defect
c7d87229(ggml-org#24233) restoredprop.integratedon HIP builds. CUDA keeps it off, with the commentTemporarily disabled due to issues with corrupted output. With it on, the scheduler may place a compute input in pinned host memory, where an H2D input write can race a graph that is still running.Two reporters bisected to that exact commit independently, from different symptoms:
-np 4 --kv-unifiedreturns another slot's response verbatim, gfx1151////////, recovering on reload; same harness scores 78-82% against cloud endpoints and ~17% hereCorruption tracks
n_ubatch < n_batch, and output is clean whenever the whole prompt lands in a single ubatch. Makingintegratedruntime-switchable on an otherwise unmodified tree gives PPL 850,121 on and 72.80 off, from one binary.This is the bug that reaches users who did nothing unusual: no environment variable, no special flags. It is separate from the unified-memory corruption in #157, and neither fix addresses the other.
Scope, checked rather than assumed
info.devices[id].integratedis set fromprop.integratedonly underGGML_USE_HIP; CUDA and MUSA get a hardfalse(ggml-cuda.cu:305-309). Thesupports_buftchange is therefore dead code off HIP.prop.integratedis false there.get_host_buffer_typenow returnsnullptrwhenGGML_CUDA_NO_PINNEDis set, on every backend. That is a consistency fix, sinceprops->host_bufferalready reportedfalsewhile the buffer type was still handed out, so a user asking for no pinned memory could still get it. The only caller in the tree,ggml-backend-meta.cpp:379, handlesnullptrexplicitly.Why this and not ggml-org#27311
ggml-org#27311 is the better end state: a ring buffer for input tensors, making host buffers correct rather than unavailable. It is an 18-commit scheduler change and currently
CONFLICTING, so it is not pinnable today. When it lands, both this and the #147 pin come out.ggml-org#25863 is the narrow version: refuse to schedule compute out of the host buffer on HIP integrated devices, while pinned memory stays available for staging. Measured at PPL 63.67 against a Vulkan reference of 64.10, where the base gives 872,006.
Verification
Not yet measured on our runner. I would rather say so than imply otherwise: the gfx1151 machine available here has no compiler, so the check has to run against a nightly that carries the pin. Once one exists,
llama-perplexityover wikitext-2 at-b 2048 -ub 512before and after, expecting the previous nightly to be the broken number and the new one to match a Vulkan reference, plus one-np 4 --kv-unifiedrun against ggml-org#25992's nonce-checked harness.Throughput is unmeasured. This changes buffer placement on a chip where both pools are the same DRAM, so a benchmark is warranted before assuming it is free, and a benchmark has to check output validity or it will reward the broken build.