ggml-cuda: use cudaMemcpyDefault in the ggml_cuda_cpy 2D fast path - #157
Open
danielhanchen wants to merge 1 commit into
Open
ggml-cuda: use cudaMemcpyDefault in the ggml_cuda_cpy 2D fast path#157danielhanchen 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
danielhanchen
added a commit
that referenced
this pull request
Aug 31, 2026
ROCm APU user with no opt-in. #157 stops the unified-memory corruption for users who have GGML_CUDA_ENABLE_UNIFIED_MEMORY set. both would apply the same hunks twice and fail the resolve, so only one can be listed. Both entries come out once a base tag carries the upstream merge. Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>
The 2D fast path added in ggml-org#25057 copies with cudaMemcpyDeviceToDevice. That argument is not a hint, it is an assertion that both pointers are device resident. Under GGML_CUDA_ENABLE_UNIFIED_MEMORY the allocator returns hipMallocManaged pages, which migrate to the host once the working set exceeds VRAM, so at high footprint the assertion is false and the copy reads the wrong memory. cudaMemcpyDefault resolves residency per pointer instead, and is a no-op when both pointers really are on the device. Neither vendors/hip.h nor vendors/musa.h mapped cudaMemcpyDefault, so the one copy kind that resolves residency was unreachable from this file on non-CUDA backends. Both shims now carry the mapping. Measured on gfx1151, 45.1 GiB model at 73% of RAM, in one job on one host: this build produced identical tokens with the variable unset and set, while the stock build in the same job reproduced the reported corruption. A revert of 25057 is also clean but drops same-type strided copies outside F32/F16/BF16/I32 onto GGML_ABORT at cpy.cu:609, losing the coverage 25057 added for the GDN recurrent snapshot with -np 4. This keeps it. Not measured against a discrete GPU or the CUDA path.
danielhanchen
force-pushed
the
fix/cpy-memcpy-default
branch
from
August 31, 2026 08:13
453cbcb to
6c6da89
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>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c6da89266
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+478
to
+479
| // GGML_CUDA_ENABLE_UNIFIED_MEMORY these buffers come from | ||
| // hipMallocManaged, and managed pages are not guaranteed device |
There was a problem hiding this comment.
In CUDA and MUSA builds, these buffers come from cudaMallocManaged and musaMallocManaged, respectively, so the backend-shared comment incorrectly states that they come from hipMallocManaged. Generalize the explanation to managed allocations and reduce the seven-line hard-wrapped comment to the repository's expected one or two concise lines.
AGENTS.md reference: AGENTS.md:L74-L78
Useful? React with 👍 / 👎.
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
One word, plus the vendor mapping that makes it compile. Replaces #155, which showed 278 files because its branch was cut from the upstream
b10687tag while targeting fork master; the change itself was always these three files.The defect
The 2D fast path added by ggml-org#25057 copies with
cudaMemcpyDeviceToDevice. That argument is not a hint, it is an assertion that both pointers are device resident, and it lets the runtime skip residency resolution.Under
GGML_CUDA_ENABLE_UNIFIED_MEMORYthe allocator returnshipMallocManagedpages. Those migrate to the host once the working set outgrows VRAM, so past a footprint threshold the assertion is false and the copy reads the wrong memory.cudaMemcpyDefaultresolves residency per pointer, and is a no-op when both pointers really are on the device.Neither
vendors/hip.hnorvendors/musa.hmappedcudaMemcpyDefault. The one copy kind that resolves residency was unreachable from this file on non-CUDA backends, which is plausibly why the defect was easy to write: on HIP the only kinds spelledcuda*were the explicit ones, so the fast path used an explicit kind because that is what compiled. The first build of this change failed withuse of undeclared identifier 'cudaMemcpyDefault'.How it was found
A release bisect over prebuilt ROCm tarballs put the corruption on one commit:
0ed235ea, b9826 clean and b9827 broken, direct parent and child, with 13 consecutive clean builds below and 5 broken above.A revert-only build then proved causation. On gfx1151, in one job on one host, with a 45.1 GiB model at 73% of RAM: the reverted build produced identical tokens with the variable unset and set, while the stock build reproduced the reported fingerprint
' Paris.,,, or or or is is is is,,,,, from from from'.Why this and not the revert
Deleting the fast path drops same-type strided copies of types outside F32/F16/BF16/I32 onto
GGML_ABORT("unsupported type combination")atcpy.cu:609. That is coverage ggml-org#25057 added for the GDN recurrent snapshot with-np 4. Changing the copy kind keeps it.Verification
Run on gfx1151, both cells in one job on one host, same model, same flags:
Paris. The capital of Germany is Berlin...47.7 tok/sb10687-mix-67dfc8bParis.,,, or or or is is is is,,,,,45.0 tok/sD2 is the load-bearing half: a build that failed to load, or a host too starved to reproduce, would also have made D1 look clean.
Trust checks on all four states:
server_readytrue, no crash, no SIGABRT, loads 6.0 / 43.6 / 18.0 / 35.1 s against a 900 s timeout. Binaries confirmed per file rather than by cell name. D1 and D2 base token vectors are identical, so the change does not perturb the path that was already correct.What is not covered
One model, one quant, one configuration, one host. No discrete GPU and no CUDA measurement, and this call site is shared with the path that was never broken. The comparable throughput pair is the two base states, 47.7 against 48.2, which is evidence against a large penalty rather than proof of none:
cudaMemcpyDefaultdoes a pointer attribute lookup per call, and a same-output benchmark is what would settle it.