cuda: block-per-row FWHT for widths 512 and up (+17% / +11% decode on folded bands) - #166
Open
bri-prism wants to merge 2 commits into
Open
cuda: block-per-row FWHT for widths 512 and up (+17% / +11% decode on folded bands)#166bri-prism wants to merge 2 commits into
bri-prism wants to merge 2 commits into
Conversation
Profiled 9B and 2B decode on an H100: the Hadamard transform was the largest single kernel, 25.8% of GPU time at block 4096 (shared-memory kernel, 13.8 us per row) and 22.5% at block 2048 (warp kernel, 7.7 us per row). At one row the warp kernel holds N/32 registers per lane and runs every stage on one warp; the shared-memory kernel synchronises twice per stage for all log2(N) stages. fwht_cuda_block runs one 256-thread block per row: stages below the warp width use shuffles, the three stages up to the block width go through shared memory, the rest stay in registers. Same butterfly and sign convention as before. Widths 64-256 keep the warp kernel; GGML_CUDA_FWHT_LEGACY=1 restores the old kernels for A/B.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is correct and covered by existing FWHT tests; only minor comment-style cleanup remains.
Pull request overview
Optimizes CUDA FWHT latency for wide rows by using one 256-thread block per row.
Changes:
- Adds a block-per-row FWHT kernel for widths 512-8192.
- Preserves legacy kernels behind
GGML_CUDA_FWHT_LEGACY.
File summaries
| File | Description |
|---|---|
ggml/src/ggml-cuda/fwht.cu |
Adds and dispatches the optimized wide-row kernel. |
Review details
Suppressed comments (1)
ggml/src/ggml-cuda/fwht.cu:254
- This launch-policy comment is hard-wrapped across four lines and includes benchmark-specific rationale, contrary to the concise, non-hard-wrapped comment convention in
AGENTS.md:66-73. Retain the dispatch rule and legacy override without the task-specific history.
// From 512 up, one block of FWHT_BLOCK_THREADS per row (fwht_cuda_block); the warp kernel
// serialised the whole transform on one warp and the shared-memory kernel synchronised every
// stage, and both were the largest single kernel of a decode step on the folded models.
// GGML_CUDA_FWHT_LEGACY=1 restores the previous kernels for A/B.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both blocks hard-wrapped prose, and the first restated the stage-by-stage implementation that the stage comments below it already document. Kept only why this kernel is selected at these shapes, and the legacy env knob. One sentence per line, none split across lines. Comments only, no change in behaviour.
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
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.
What
One 256-thread block per row for the Hadamard transform at widths 512 and above, replacing the warp-per-row kernel (512 to 2048) and the shared-memory kernel (4096, 8192). Stages below the warp width use shuffles, the three stages up to the block width go through shared memory, the stages above it stay in registers. Same butterfly and the same sign convention as both old kernels, so results are unchanged. Widths 64 to 256 keep the warp kernel.
GGML_CUDA_FWHT_LEGACY=1restores the previous kernels for A/B.Why
An
nsyskernel summary of a 64-token decode on an H100 PCIe put the transform first among all kernels on the folded bands, ahead of the weight mat-vecs:At one row the warp kernel holds N/32 floats per lane and runs all log2(N) stages on a single warp while the rest of the GPU idles; the shared-memory kernel synchronises twice per stage for all twelve stages. This is the same latency structure the Metal side had before the threadgroup-FWHT change.
Verification (H100 PCIe, CUDA 12.8, base
8c0170d19, separate worktrees,-p 512 -n 128 -r 3, two interleaved passes)test-backend-ops test -b CUDA0 -o MUL_MAT_HADAMARD: exit 0, 27/27 passed.GGML_CUDA_FWHT_LEGACY=1The legacy-env column is the same binary with the old kernels, and it lands back on the base numbers, so the delta is the kernel and not the build. Prefill spread on this card is wide (pp512 error bars are 900 to 4,700), so read the prefill column as "not worse".