ggml-cuda: avoid direct ROCm_Host compute on HIP integrated GPUs (port of ggml-org#25863) - #158
Open
danielhanchen wants to merge 1 commit into
Open
ggml-cuda: avoid direct ROCm_Host compute on HIP integrated GPUs (port of ggml-org#25863)#158danielhanchen wants to merge 1 commit into
danielhanchen 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. |
This was referenced Aug 31, 2026
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.
danielhanchen
force-pushed
the
port/25863-hip-integrated-host-buft
branch
from
August 31, 2026 08:14
bfe2398 to
abfc45b
Compare
danielhanchen
added a commit
that referenced
this pull request
Aug 31, 2026
…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>
danielhanchen
added a commit
that referenced
this pull request
Aug 31, 2026
…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>
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
1 participant
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.
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.