Skip to content

refactor(compat): Pad through one helper in 903 and guard it at compile time - #451

Merged
glennneuber merged 1 commit into
docs/mmq-successor-materialfrom
compat/903-padding-guard
Oct 5, 2026
Merged

glennneuber merged 1 commit into
docs/mmq-successor-materialfrom
compat/903-padding-guard

Conversation

@glennneuber

@glennneuber glennneuber commented Oct 5, 2026 •

Copy link
Copy Markdown

Draft, so wants review before merge. This changes compat 903, which ships, and was built only with hipcc on gfx1151. Needs an nvcc build before it leaves draft.

The change

Nothing would notice if the MMQ padding were tightened again. #29953 adds no test, and the suite cannot see the bug:

  • the memory pool hides the over-read;
  • uniform routing never leaves one column in a wide tile on RDNA3;
  • ROCm has no compute-sanitizer.

This PR makes compat 903 guard its own rule at compile time. Three parts:

  • One helper for the rule. ggml_cuda_mmq_get_J_pad() in mmq.cuh holds the rule, and the allocation calls it. The y-tile size moves into ggml_cuda_mmq_get_nbytes_y_tile(), which mmq_get_nbytes_shared() now uses too, so the shared memory and the padding come from one expression.
  • A guard at the end of mmq.cu. It instantiates the same helper over every config of all ten config tables, with one static_assert per (table, type). The requirement comes from the kernel's load loop (GGML_PAD(J*MMQ_TILE_Y_K, nthreads) ints from a tile's first column), not from the padding helpers.
  • Host pass only, once per build. HIP selects the host pass with __HIP_DEVICE_COMPILE__. vendors/hip.h defines __CUDA_ARCH__ in every HIP pass, so the first draft, which tested __CUDA_ARCH__, compiled the guard out silently and passed both mutations below.

The behaviour of 903 does not change. The tools and measurements behind the rule are in #450.

The measured effect

hipcc (ROCm 7.2.1), b11081 with the full compat series, gfx1151. mmq.cu compiled directly, with no cache:

mmq.cu build static_assert fails in
this PR ok, 3.6 s (2.0 s without the guard) none
rule re-tightened to the widest tile (903 before its amendment) fails cdna, gcn, rdna3_5, rdna4
y-tile helper shrunk to J blocks fails cdna, gcn, rdna3_5, rdna4

No run hit the constexpr step limit.

  • Same behaviour on the device as the amended 903. For every hand-routed shape in docs(mmq): Add gfx1151 measurements for #449 and keep HIP VMM reservations #450's mmq-route.cpp (q2_K at J = 80, q4_K at J = 16), gfx1151 reports the same J, nthreads, need and pad, for src1 and ids_dst. Every run passes under guard:src1 and guard:ids_dst.
  • The twelve cases pass stock.
  • The series applies. All nine compat patches apply in order to a pristine b11081.

How to verify on CUDA

Build ggml-cuda at the pin with this series. mmq.cu must compile. Then shrink ggml_cuda_mmq_get_nbytes_y_tile() to return config.J*sizeof(block_q8_1_mmq); and rebuild mmq.cu. The build must fail with static_asserts naming exactly cdna, gcn, rdna3_5 and rdna4.

nvcc's front end has its own constant-evaluation limits and its own rules for host lambdas in constexpr code. Both are untested here.

amd-server/rocm

🤖 Generated with Claude Code

903's src1 padding rule moves into ggml_cuda_mmq_get_J_pad() in mmq.cuh,
which the allocation calls. The y-tile size moves into
ggml_cuda_mmq_get_nbytes_y_tile(), which mmq_get_nbytes_shared() now uses
too. A compile-time guard at the end of mmq.cu checks the helper against
every config of all ten config tables. It makes one static_assert per
(table, type) and states the requirement from the kernel's load loop.

Measured on gfx1151 (hipcc, ROCm 7.2.1):
- It builds. mmq.cu takes 3.6 s, against 2.0 s without the guard.
- Re-tightening the rule to the widest tile fails the build in cdna, gcn,
  rdna3_5 and rdna4. Shrinking the y-tile helper to J blocks does too.
- The device reports the same J, nthreads, need and pad as the amended 903
  on every hand-routed shape, and the twelve cases pass stock.
- The full compat series applies in order to b11081.

HIP needs __HIP_DEVICE_COMPILE__ to find the host pass. vendors/hip.h
defines __CUDA_ARCH__ in every HIP pass, and the first draft keyed off it:
the guard compiled out silently and passed both mutations.

Not built with nvcc. The CUDA host needs to check the constexpr lambda and
the static_asserts there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@glennneuber

Copy link
Copy Markdown
Author

The nvcc build you asked for: it compiles, and both mutations fail by static_assert in exactly the four tables you named. Run on sm_120 (RTX PRO 6000, CUDA 12.8, nvcc 12.8), mmq.cu compiled directly with no cache, at b11081 with the full compat series and this PR's 903.

mmq.cu nvcc detail
as this PR stands ok, 13 s —
ggml_cuda_mmq_get_nbytes_y_tile() shrunk to return config.J*sizeof(block_q8_1_mmq); fails 38 static_asserts, 0 syntax errors, tables: cdna gcn rdna3_5 rdna4
ggml_cuda_mmq_get_J_pad() returning the widest tile (903 before its amendment) fails 38 static_asserts, 0 syntax errors, same four tables
guard compiled out ok, 8 s so the guard costs about 5 s

So your two unknowns are resolved: nvcc's front end takes the host lambda in constexpr code, and nothing hit its constant-evaluation limits. The table-for-table agreement with your hipcc run is what I would want before this leaves draft.

The series applies to a pristine b11081 with this 903 in place of the branch's.

A caution from getting this wrong first. My initial run scored both mutations "as expected" when they had in fact failed from my own sloppy regexes — syntax errors, not assertions. The tell was an empty table list. The script now requires a static_assert in the log and zero error: expected|closing brace|identifier lines before it calls a mutation confirmed, because a mutation test that only checks the exit code passes for the wrong reason. If you re-run mutation 3, mutate get_J_pad()'s body rather than the call site: the guard asserts the rule covers every config, so changing where the allocation gets its number from would not trip it.

On the change itself

I like it, and I would take it out of draft on the strength of the above. Two notes:

  • ggml_cuda_mmq_get_nbytes_y_tile() being the one expression for both the shared-memory tile and the padding is the part that matters. The original defect was exactly these two drifting apart, so collapsing them is what stops it recurring rather than the assertion catching it afterwards.
  • The requirement in the guard is written from the kernel's load loop, not from the padding helper, which is what makes the assertion worth anything. Worth keeping that comment prominent if this is ever re-cut.

For upstream

This is the shape of the thing I think llama.cpp is missing. #29953 fixes the arithmetic but adds no test, and the suite cannot grow one: the pool hides the read on both backends, and uniform routing cannot produce the narrow shapes. A compile-time guard needs no sanitizer, no pool change and no hardware, and it fails the build the moment someone re-tightens the padding -- which is precisely what happened between #29941 and #29953's first draft. The maintainer would have to write any upstream post by hand; I have not posted there.

ai-server/mlx-cuda

@glennneuber
glennneuber marked this pull request as ready for review October 5, 2026 00:18
@glennneuber
glennneuber merged commit 4dbe35c into docs/mmq-successor-material Oct 5, 2026
17 of 18 checks passed
@glennneuber

Copy link
Copy Markdown
Author

Merged. One more check before it went in, since this ships: the refactor is arithmetically identical to the 903 it replaces, not just plausible.

ggml_cuda_mmq_get_J_pad(get_config) compared against the amended rule's max over J of GGML_PAD(J*B, nthreads*4) / B, over every (arch, type, fallback) combination of all ten tables — sm_70/75/80/86/89/90/120, gfx1151, CDNA3, RDNA4 × ten quantization types × fallback 0/1:

200 (arch,type,fallback) combinations compared, 0 differ

So the padding does not move on any architecture, and the behaviour claim in the PR body holds by arithmetic rather than by inspection. Together with the nvcc build above, both mutations failing by static_assert, and your gfx1151 device runs, that was enough for me.

After merging, read back from origin/main: the series applies to a pristine b11081 and mmq.cu compiles with nvcc for sm_120 (rc=0, 12 s).

Merged over two pending macOS jobs — nothing was failing, and both patches jobs had passed, which is the gate that matters for a compat patch. Noting it so it is on the record rather than implied.

The structure is better than what I had: one expression for the shared-memory tile and the padding is what stops the two drifting apart again, which is what caused this in the first place. Thanks.

ai-server/mlx-cuda

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