Skip to content

ggml-cuda: stop scheduling compute onto the host buffer on HIP integrated GPUs - #145

Closed
danielhanchen wants to merge 1 commit into
masterfrom
hip/no-host-buft-on-integrated
Closed

ggml-cuda: stop scheduling compute onto the host buffer on HIP integrated GPUs#145
danielhanchen wants to merge 1 commit into
masterfrom
hip/no-host-buft-on-integrated

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

Overview

Carries ggml-org#25863 so our ROCm prebuilts stop scheduling compute onto the pinned host buffer on HIP integrated GPUs.

The asymmetry is one #if in ggml-cuda.cu:

#if defined(GGML_USE_HIP)
        info.devices[id].integrated = prop.integrated;
#else
        info.devices[id].integrated = false; // Temporarily disabled due to issues with corrupted output (e.g. #15034)
#endif

CUDA disables the flag because it produced corrupted output. HIP does not. On an APU that turns on a host-buffer path where the scheduler can place a compute input in pinned host memory, and an H2D input write can then race a graph that is still running.

Why now

This is the defect behind the largest group of Strix Halo reports, and all of them are on builds we ship:

  • ggml-org#25992 bisected it to c7d87229 ("ggml-cuda : restore prop.integrated on HIP builds", ggml-org#24233), with a clean parent, on gfx1151. Symptom: -np 4 --kv-unified returns another request's response verbatim.
  • ggml-org#27506 reached the same commit independently from a different symptom, a perplexity explosion from b10040: 7.72 to 3024 on Llama-3.2-3B. Confirmed on three separate gfx1151 machines, Vulkan clean on each.
  • ggml-org#27579 and ggml-org#27556: HIP wrong, Vulkan correct at the identical commit on the same box.
  • lemonade-sdk/llamacpp-rocm#123: "various models start becoming somewhat to fully broken from b1298", multiple gfx1151 reporters, upstream Vulkan flawless.

The rule those threads established is that corruption tracks n_ubatch < n_batch and is clean whenever the whole prompt lands in a single ubatch. Making integrated runtime-switchable on an otherwise unmodified tree gives PPL 850,121 with it on and 72.80 with it off, from the same binary.

Two points worth carrying into how we test this. One reporter ran a greedy completion canary clean for forty minutes against a backend that was numerically broken throughout, so tok/s and HTTP 200 are not evidence of correctness here. The same reporter measured Vulkan at 24% faster decode on this chip, so there is no throughput argument for staying on the ROCm build while this is open.

What the change does

Three call sites, matching ggml-org#25863:

  • ggml_backend_cuda_host_buffer_supported() factors out the existing GGML_CUDA_NO_PINNED test.
  • ggml_backend_cuda_device_supports_cuda_host_buft() returns false for HIP integrated devices.
  • ggml_backend_cuda_device_supports_buft() consults it, so the scheduler stops placing compute inputs on the host buffer type.

Pinned host memory stays available for staging. Only compute directly out of it is refused, which is the narrowest change that closes the reports. Discrete HIP and every CUDA path are untouched, since integrated is already false there.

Alternatives considered

ggml-org#27311 is the general fix, a ring buffer for input tensors that makes host buffers correct rather than unavailable. It is the better end state and is what we should follow, but it is a scheduler change with four pending reviews and no approvals. ggml-org#25863 is small enough to carry now and is measured at PPL 63.67 against a Vulkan reference of 64.10 where the base gives 872,006.

Verification

Not built or measured here, and I want to be plain about that. The gfx1151 CI runner available to me has no compiler and this needs the ROCm toolchain, so the prebuilt legs in this repo are the first real check. Before merging I would want:

I can drive the second and third on the Strix Halo runner once there is a build.

…ated GPUs

Carries ggml-org#25863. On HIP the device info sets
integrated = prop.integrated, while the CUDA branch of the same #if keeps it
false 'due to issues with corrupted output'. That asymmetry re-enables direct
host-buffer compute on APUs, which is the path the gfx1151 corruption reports
bisect to. Pinned staging stays available; only compute directly out of the
host buffer is refused.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-30T11:15:01.919205Z 043f806 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@danielhanchen

Copy link
Copy Markdown
Member Author

Closing this in favour of #147.

I had the shipping model wrong when I opened it. The nightly tree is the upstream tag plus scripts/unsloth/pr-set.json, not fork master, so merging this would have left every ROCm APU user on exactly the same broken binary while looking like the fix had landed. _doc in that file says so directly: merging an unslothai PR into fork master drops it from the nightly.

#147 pins ggml-org#25863 itself, which is the same change, open and MERGEABLE upstream at ce82541. Pinning upstream rather than carrying a cherry-pick here also means the entry retires cleanly once a base tag contains the work, instead of turning into a conflicting re-application of code the base already has.

The evidence and the reasoning are all restated on #147.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant