vulkan: fix COL2IM_1D dispatch for large tensors - #542
AdityaAWaghmare wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Dispatch-limit handling and regression coverage remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes Vulkan COL2IM_1D failures on large tensors by dispatching T_out and OC separately instead of flattening them into X.
Changes:
- Maps shader invocations to
(t_out, oc). - Dispatches
{T_out, OC, 1}while preserving output layout and push constants.
File summaries
| File | Summary |
|---|---|
external/ggml/src/ggml-vulkan/vulkan-shaders/col2im_1d.comp |
Implements 2-D indexing and bounds checks. |
external/ggml/src/ggml-vulkan/ggml-vulkan.cpp |
Updates dispatch dimensions. Findings: critical (1 vote)—dimensions can still exceed device limits; moderate (2 votes)—add boundary regression coverage; moderate (1 vote)—avoid excessive padding for short outputs. |
Review details
Suppressed comments (1)
external/ggml/src/ggml-vulkan/ggml-vulkan.cpp:10384
- The 2-D dispatch changes the amount of work for short outputs: it launches
ceil(T_out / 256) * 256 * OCinvocations, versusceil(T_out * OC / 256) * 256before. ForT_out < 256this can be up to 256× more threads (for example,T_out = 1launches one 256-thread workgroup per channel, with only one lane doing useful work). Consider retaining the flattened path when it is within the device X limit and selecting the 2-D path only for overflowing dispatches, or otherwise avoiding this per-row padding.
elements = { uint32_t(dst->ne[0]), uint32_t(dst->ne[1]), 1 };
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| const uint32_t total = (uint32_t)(dst->ne[0] * dst->ne[1]); | ||
| elements = { total, 1, 1 }; | ||
| elements = { uint32_t(dst->ne[0]), uint32_t(dst->ne[1]), 1 }; |
| { | ||
| const uint32_t total = (uint32_t)(dst->ne[0] * dst->ne[1]); | ||
| elements = { total, 1, 1 }; | ||
| elements = { uint32_t(dst->ne[0]), uint32_t(dst->ne[1]), 1 }; |
|
A couple of points for further discussion: The 2D dispatch approach here follows the upstream llama.cpp implementation of GGML_OP_COL2IM_1D, introduced in ggml-org/llama.cpp#24425 (commit ad39cca). The upstream implementation dispatches T_out and OC across X/Y respectively, with the same local_size_x = 256 approach used here. Regarding the possibility of still exceeding maxComputeWorkGroupCount: that's a general property of dispatch dimensions and isn't specific to this change. The existing assertion already checks all three dispatch dimensions. The problem reproduced in this issue was specifically the flattened X dispatch: for the failing workload it required 78,000 X workgroups, exceeding the 65,535 limit on the Intel device. Splitting the logical dimensions gives (204, 384, 1) for that workload. The concern about extra invocations for small T_out is valid and could potentially be optimized with a different/adaptive dispatch strategy. However, I kept this PR aligned with the upstream implementation and focused it on the correctness issue that caused the crash. Happy to discuss whether an adaptive approach would be preferable here. |
|
@AdityaAWaghmare Thank you for digging deep and finding the root cause. I need more time to test it because it changes dispatch cost for every shape and have performance implications. It being upstream is a good sign, but we've tried bumping the GGML version before and couldn’t move forward because of performance regressions. Could you try another apporach: keep shader untouched and cap the X dispatch size on the host side: E.g. The change is much smaller. The existing shader already has a grid-stride loop based on the actual dispatched workgroup count, so capping X should still cover the full output while preserving the existing dispatch for shapes that fit. Could you test this against the failing workload and compare correctness and performance around the dispatch boundary? |
Problem
The Vulkan implementation of
GGML_OP_COL2IM_1Dflattened the[T_out, OC]output into a single X dispatch dimension.For sufficiently large tensors this can exceed
maxComputeWorkGroupCount[0]on Vulkan devices with a lower Xworkgroup-count limit, triggering the dispatch assertion.
This was first reproduced on Intel ADL GT2 with PocketTTS and
Qwen3-TTS for longer inputs, but the issue is not Intel-specific.
Any Vulkan device whose
maxComputeWorkGroupCount[0]is smaller thanthe required flattened dispatch can hit the same problem.
For example:
T_out = 52000OC = 38419,968,000256ceil(19,968,000 / 256) = 78,000This exceeds a device limit of
65,535.Fix
Dispatch the two logical output dimensions separately:
T_outOCThe shader now maps
gl_GlobalInvocationID.xtot_outandgl_GlobalInvocationID.ytooc.This changes the example dispatch from:
to:
while preserving the existing output layout and COL2IM computation.
The existing push-constant layout is preserved.
This avoids relying on a large flattened X dispatch and makes the
operation portable across Vulkan devices with different
maxComputeWorkGroupCountlimits.Testing
Tested with Vulkan on Intel ADL GT2:
Both previously reproduced the
maxComputeWorkGroupCountassertion.Also tested PocketTTS on NVIDIA RTX 3050 Vulkan:
Qwen3-TTS on the RTX 3050 could not be tested because the 4 GB VRAM
configuration ran out of device memory.