Repository navigation
ggml : add alloc_buffer_n to buffer type interface - #23671
Conversation
Port ggml-org/llama.cpp#23671 into advanced-gguf-quantizer as one local integration commit. Includes upstream commits 0743dce657dc9901ee56734dbce571cf0a6abb8d (add alloc_buffer_n to buffer type interface) and 3f9bcab883c84c6336e100ce5d7408bb4bf47ac9 (fix cur_buf_size after buffer flush).
3f9bcab to
7c7be0f
Compare
d2ebf49 to
a75c427
Compare
|
|
||
| static ggml_backend_buffer_t ggml_backend_alloc_ctx_tensors_from_buft_impl( | ||
| struct ggml_context * ctx, ggml_backend_buffer_type_t buft, size_t * nbytes_total, bool no_alloc) { | ||
| // TODO [TAG_ALLOC_SHARED_BUFFER_SPLIT]: reuse shared buffer-splitting logic from ggml_backend_buft_alloc_buffer_n_default |
There was a problem hiding this comment.
If we allow ggml backends to implement their own logic for allocating ggml tensors then I think we have to also make the corresponding function for projecting the memory use part of the ggml backend API. Otherwise a backend implementing its own memory allocation will automatically break -fit. This is in essence why as of right now ggml_backend_buft_get_alloc_size is broken for the meta backend and why -fit and -sm tensor are incompatible. To the code in fit.cpp it is currently not clear how e.g. a logical meta buffer maps to physical buffers.
The API to make this work will I think need to be that a logical ggml backend buffer type gets some property like n_physical to represent how many physical ggml backend buffer types it contains (1 by default). When fetching the (projected) allocated size, add functions that return n_physical values, so effectively a breakdown by physical buffer types. We can keep the current functions for convenience but change their semantic meaning to be that they return the aggregate size of all physical buffer types.
There was a problem hiding this comment.
And I guess for the fit + -sm tensor we cannot work with just knowing the total size across all the physical buffers? Because if we could, we can just add the respective get_alloc_size_n.
Can the fit logic not determine the physical split of the total size based on the split states information that it should have knowledge of?
There was a problem hiding this comment.
It depends on how large our tolerance for imprecision is. We can approximate the memory use per backend from the total sum + the tensor split. For most cases I expect this to be within ~5% and in principle workable (since we have a 1 GiB margin by default). But we will not be able to give users information with MiB precision as we do otherwise.
I see this as less of a llama.cpp issue and more as a ggml issue. If we have a function that is supposed to tell user code how much memory will be allocated I think it's a comparatively poorer experience for developers if the result they get is only approximately correct.
There was a problem hiding this comment.
We can in principle also treat the meta backend as a special case that needs to be treated separately in the user code. The question is whether we expect to have more such backend buffer types in the future that would warrant a more general API.
There was a problem hiding this comment.
Adding a get_alloc_size_n call is fine and is basically the right thing to do in order to make the ggml_backend_alloc_ctx_tensors_from_buft_size work correctly in general, as you correctly noted. So I think I'll follow up on that.
Whether the ggml API should provide a fine-grained information about the sub-buffers - I am not yet convinced. Querying for n_physical values loses generality and does not seem like a good design. A better option would be to have a call that basically asks "how much is the allocation size of these tensors on device X using this buffer type?". But still, I am not sure we really want such level of specificity.
If we can make the fit logic work with a small tolerance, I would say it's good enough for now. And as the meta backend gets more adopted, we can reconsider.
There was a problem hiding this comment.
Let's for now only add the functionality that we are sure we will definitely need.
There was a problem hiding this comment.
In principle, (almost) all of the functionality that I would need in fit.cpp is already exposed in ggml-backend-impl.h. I think we at some point had a header like ggml-ext.h for unstable ggml functionality that we removed again for some reason. What was the problem again?
There was a problem hiding this comment.
The ggml-ext.h is not compatible in cases where a downstream builds nightly llama.cpp and uses a system-wide ggml binaries (instead of the local ggml copy from llama.cpp). This was the case for Homebrew back then: #21869. Now that we have a semantic versioning in place, this is no longer a problem, since the downstreams are supposed to build from the stable tags. For example, Homebrew is already doing that.
max-krasnyansky
left a comment
There was a problem hiding this comment.
Looks good to me as a general improvement.
For ggml-hexagon, it would be useful to allocate different buffers for different tensors from the list. With the addition of 64-bit DMA support, we now have two buffer flavors: regular and extended (64-bit mapping). Currently, all buffers with usage=weight are mapped as extended, which forces kernels to use DMA for every tensor allocated within them. I’ve already updated most kernels to handle this, so it’s not a blocker, but it would be helpful to have per-tensor flexibility—e.g., if a tensor is too small or its kernel isn't ready for DMA, we could route it to a different buffer rather than bundling it with the rest.
514cd02 to
6662b12
Compare
|
Ah. I see that it's possible to return the multi-buffer where different tensors map to different sub-buffer. Thanks for adding that alloc-plan based implementation. Let me know if I misread that :) |
6662b12 to
aa7ee0b
Compare
Yes. We don't currently have a specific application of this new API apart from the meta backend. So would be nice in case you find a use case in the hexagon backend in order to give us more confidence that this change is worth it. |
Sounds good. I'll give that a shot a bit later today. |
| } | ||
| struct ggml_tensor ** tensors = (struct ggml_tensor **) malloc(n * sizeof(struct ggml_tensor *)); | ||
| if (tensors == NULL) { | ||
| return NULL; |
There was a problem hiding this comment.
If we return NULL here ggml_backend_alloc_ctx_tensors_from_buft_size will return 0 which is likely to cause weird bugs in user code. I think this needs a print to one of the ggml logs to make debugging easier. Preferably also document in the header that NULL and 0 are returned in case of an error.
Sorry for the delay. Seems to work as expected. Here is the first cut where we split large tensors into standalone buffers |
|
@ggerganov @JohannesGaessler any reason we're not merging this yet? |
Add alloc_buffer_n method to ggml_backend_buffer_type_i interface, with a public API ggml_backend_buft_alloc_buffer_n. - Default implementation in ggml-backend.cpp handles multi-buffer splitting and tensor allocation via ggml_tallocr - Meta buffer type provides custom implementation that creates per-device sub-contexts and delegates to simple buffer types - ggml_backend_alloc_ctx_tensors_from_buft now collects tensors into a list and delegates to the new API - Remove temporary ggml_backend_meta_alloc_ctx_tensors_from_buft - Add NULL alloc_buffer_n to all existing buffer type interfaces (cpu, metal, openvino, hexagon, webgpu, zdnn, virtgpu, repack) Assisted-by: llama.cpp:local pi
Assisted-by: pi:llama.cpp/DeepSeek-V4-Flash-Vision-Exp
Assisted-by: pi:llama.cpp/DeepSeek-V4-Flash-Vision-Exp
Assisted-by: pi:llama.cpp/Qwen3.8-27B
- restore GGML_LOG_ERROR on buffer alloc / tensor init failure in the default impl (name the failing tensor) - check the malloc result and drop the _impl indirection in ggml_backend_alloc_ctx_tensors_from_buft - remove comments that restate the code - fix the TAG_ALLOC_SHARED_BUFFER_SPLIT typo Assisted-by: pi:llama.cpp/Qwen3.8-27B
- Add ggml_backend_buft_get_alloc_size_n public API - Add optional get_alloc_size_n callback to ggml_backend_buffer_type_i - Share tensor->buffer planning between alloc_buffer_n default and get_alloc_size_n default - Replace unchecked realloc with std::vector in alloc_buffer_n default - Make ggml_backend_alloc_ctx_tensors_from_buft_size use the new API - Add test-alloc coverage for get_alloc_size_n Assisted-by: pi:llama.cpp/DeepSeek-V4-Flash-Vision-Exp
aa7ee0b to
d211167
Compare
|
Sorry for the delay - I wanted to also implement |
Clean upstream sync past 254b177. Relevant to the fork's deployment: - 4e2713c qwen4exp : optimize mask constructions (ggml-org#29824) - 631109b ggml : add alloc_buffer_n to the buffer type interface (ggml-org#23671) plus sycl / vulkan / opencl kernel work, llama warning and abort cleanups, and a new server /v1/systemone API (ggml-org#29818). Why it matters here: adopting upstream's Qwen4Exp MTP (ggml-org#29761) costs roughly +2.6 GB of compute buffers per device on the Qwen3.8-Flash-Next route, because the fixed attention path (ggml-org#29751) builds a k-pool input per QSA layer and makes the indexer cache track the attention cache cell for cell. ggml-org#29824 reduces the mask construction cost of that path. Fork-own work (RAM prompt-cache retention, selective CUDA P2P transport, server prefill/decode phase isolation, DFlash M-RoPE inference, docs, tests) is unchanged.
Merge the latest ggml-dx12-main work (linalg / flash-attn shader set, autotune header, linalg-bench tooling) and adapt all three DX12 backends to the buffer-type interface change from 631109b (ggml-org#23671), which inserted the optional alloc_buffer_n and get_alloc_size_n members and bumped GGML_BACKEND_API_VERSION from 2 to 3. - ggml-dx12, ggml-dx12x, ggml-dx12-main: add the two new nullptr slots to dx12_buffer_type_interface so the positional initializers line up with the new member order. Both new members are optional with documented defaults, so behaviour is unchanged. - ggml-dx12-main: forward-declare ggml_backend_dx12_set_env_refresh and ggml_backend_dx12_set_flag_sink ahead of dx12_reg_get_proc_address. They are defined later in the same file and are deliberately absent from ggml/include/ggml-dx12.h, being reached via get_proc_address. graph_optimize is nullptr in ggml-dx12 and ggml-dx12x, so upstream's signature change to that callback affected only ggml-dx12-main. Build-verified with Ninja/Release under MSVC: GGML_DX12_MAIN=ON, GGML_DX12=ON and GGML_DX12X=ON each configure and compile cleanly with zero warnings in the DX12 sources. Not yet exercised on device. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d7f2fcd-a611-433c-a522-a341fb127413
Flow-down of b43b8c1, which merges the latest ggml-dx12-main work and adapts all three DX12 backends to the buffer-type interface change from 631109b (ggml-org#23671) that bumped GGML_BACKEND_API_VERSION from 2 to 3. ggml/src/ggml-dx12, ggml-dx12-main and ggml-dx12x are maintained on hv/b612_100326 and mirrored here verbatim, so the three directories are replaced wholesale rather than merged. Their tree hashes match b43b8c1 exactly. No files outside ggml/src/ggml-dx12* are touched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d7f2fcd-a611-433c-a522-a341fb127413
* ggml : add `alloc_buffer_n` to buffer type interface Add alloc_buffer_n method to ggml_backend_buffer_type_i interface, with a public API ggml_backend_buft_alloc_buffer_n. - Default implementation in ggml-backend.cpp handles multi-buffer splitting and tensor allocation via ggml_tallocr - Meta buffer type provides custom implementation that creates per-device sub-contexts and delegates to simple buffer types - ggml_backend_alloc_ctx_tensors_from_buft now collects tensors into a list and delegates to the new API - Remove temporary ggml_backend_meta_alloc_ctx_tensors_from_buft - Add NULL alloc_buffer_n to all existing buffer type interfaces (cpu, metal, openvino, hexagon, webgpu, zdnn, virtgpu, repack) Assisted-by: llama.cpp:local pi * cont : fix `cur_buf_size` init after flushing a buffer * ggml : add TODO tag for shared buffer split logic Assisted-by: pi:llama.cpp/DeepSeek-V4-Flash-Vision-Exp * tests : add alloc_buffer_n coverage Assisted-by: pi:llama.cpp/DeepSeek-V4-Flash-Vision-Exp * cont : fix compile warnings * tests : add descriptions for alloc_buffer_n tests Assisted-by: pi:llama.cpp/Qwen3.8-27B * ggml : address review comments on alloc_buffer_n - restore GGML_LOG_ERROR on buffer alloc / tensor init failure in the default impl (name the failing tensor) - check the malloc result and drop the _impl indirection in ggml_backend_alloc_ctx_tensors_from_buft - remove comments that restate the code - fix the TAG_ALLOC_SHARED_BUFFER_SPLIT typo Assisted-by: pi:llama.cpp/Qwen3.8-27B * ggml : add get_alloc_size_n to buffer type interface - Add ggml_backend_buft_get_alloc_size_n public API - Add optional get_alloc_size_n callback to ggml_backend_buffer_type_i - Share tensor->buffer planning between alloc_buffer_n default and get_alloc_size_n default - Replace unchecked realloc with std::vector in alloc_buffer_n default - Make ggml_backend_alloc_ctx_tensors_from_buft_size use the new API - Add test-alloc coverage for get_alloc_size_n Assisted-by: pi:llama.cpp/DeepSeek-V4-Flash-Vision-Exp * cont : report malloc failure
Overview
cont #19378
The
ggml_backend_meta_alloc_ctx_tensors_from_buftwas a temporary workaround. This patch avoids the function by extending the buffer type interface with an API that allocates a backend buffer from a list of tensors. This decouples theggml-allocfrom meta backend specifics and allows more flexible buffer allocations in the future.Requirements