refactor(models): share the short-conv decode helper and audit L=1 conv dispatch across SSM/hybrid decode paths#753
Merged
Conversation
Lift the #751 single-step short-conv decode helpers (`build_conv_decode_weight` + `short_conv_decode_step`) out of `src/models/lfm2.rs` into a new shared `src/models/conv_decode.rs`, so the SSM / hybrid families that run the same rolling-window pad + depthwise `conv1d` every decode step (mamba2, falcon-h1, granite-4.0-h, mamba, jamba, plamo2, nemotron-h, kimi linear, qwen3.5 linear-attention) can reuse the one implementation (issue #752). The helpers are re-exported at `models::lfm2::{build_conv_decode_weight, short_conv_decode_step}` so LFM2's `ShortConv` and its existing 11 unit tests are unchanged. This is a pure move: behavior is identical. New `conv_decode_tests.rs` adds generalized channel- and tap-asymmetric parity tests (f32 + bf16, kernel widths 3 and 4, plus a `silu(bias + conv)` end-to-end variant) that pin the elementwise decode step against `conv1d` without loading a checkpoint. Validated: `cargo test --lib --features cuda models::conv_decode` (5 passed) and `models::lfm2` (11 passed, unmodified). Refs #752
Add the #752 measurement outcome to the 2026-07-12 GB10 notes. Every decode-path depthwise-conv family beyond LFM2 was profiled under nsys on a warm CUDA decode and runs on the fast `conv1d_c1_k1_nhwc` cuDNN engine, not the per-channel `convolve_common_engine` that regressed LFM2 in #748: mamba2 (bf16 130m + 1.3b-4bit), falcon-mamba-7b, falcon-h1-tiny, granite-4.0-h-350m, jamba, plamo-2-1b (f32), qwen3.5-0.8b (covers gated-delta), and nemotron-h-30b. The proof is the launch count: conv instances equal (conv layers x decode tokens), one launch per op, versus one-per-channel for the slow engine. No family needs the elementwise adaptation. Correct the PR #751 hypothesis on the record: the gemma-4-26b-a4b-it(-qat) and glm4-flash-4bit sweep drops are NOT the conv-dispatch pattern. Neither is a conv architecture (`gemma4` MoE and `glm4_moe_lite`; neither calls `conv1d`), so their decode conv cannot hit the grouped-conv engine; those drops need separate attribution. Annotate the mamba2-130m row: its conv is on the fast engine, so the -10.4% sweep delta is environmental, not conv dispatch. Refs #752
…57c66cac Apply the two LOW nits from PR #753 review. The conv_decode_tests bf16 tolerance comment attributed the sqrt(channels) scaling to the reduce accumulating more terms, but the reduce sums only over the kernel taps (3-4), independent of channel count; reword it to the real reason, an order-statistic bound on the max of independent bf16 rounding errors across channels. The model_tests_gb10.md audit paragraph recording the REFUTED SSM/hybrid conv-dispatch verdicts (#752) now names the MLX pin (`57c66cac` / 0.32.1) the measurements were taken on, since that pin's CUDA conv dispatch heuristic is what produced the lfm2 slow path, so future readers on a newer MLX know the verdicts are point-in-time. Validation: - cargo test --lib models::conv_decode --features cuda: 5 passed - cargo fmt on both touched files (no additional changes) Refs #752
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.
Summary
Follow-up to #748 / PR #751 (
3fa2789). That fix found MLX 0.32.1's CUDA backend dispatches a single-output-position (L=1) bf16 depthwiseconv1dto cuDNN's generic per-channel engine (convolve_common_engine_float_NHWC, one launch per channel), which regressed LFM2 decode ~10x, and replaced the L=1 step with a broadcast multiply plus axis sum. This PR does two things: (1) lifts the #751 decode helpers into a shared module so any family can adopt them in one line, and (2) measures every other decode-path depthwise-conv family on GB10 to see whether the same slow dispatch recurs.It does not. Every SSM / hybrid family already runs its L=1 decode conv on the fast
conv1d_c1_k1_nhwccuDNN engine, so none is adapted. The result is a pure refactor plus a documented, measurement-driven negative result.What changed
src/models/conv_decode.rs(new): sharedbuild_conv_decode_weight+short_conv_decode_step, lifted verbatim fromlfm2.rs.src/models/lfm2.rsre-exports them at the original path (pub(crate) use), soShortConvand the existing 11 lfm2 tests are unchanged. Pure move, behavior identical (lfm2-350m-8bit greedy output byte-identical: "Hello! I'm doing well, thanks. How about you?", 13 tokens, and decode still elementwise at 393-ish tok/s).src/models/conv_decode_tests.rs(new): generalized channel- and tap-asymmetric parity tests (f32 + bf16, kernel widths 3 and 4, plus asilu(bias + conv)end-to-end variant) pinning the elementwise decode step againstconv1dwithout a checkpoint.src/models/mod.rs: wireconv_decodeunder the shared modules andconv_decode_testsunder the test modules.docs/benchmark_results/model_tests_gb10.md: record the audit outcome, annotate the mamba2-130m row, and correct the PR fix(cuda): restore lfm2-350m-8bit decode via elementwise short conv #751 hypothesis on the gemma-4 / glm4-flash drops.Method
Each family with a local checkpoint was profiled under
nsyson a warm GB10 CUDA decode (mlxcel generate ... -n 24 --profile,MLX_USE_CUDA_GRAPHS=0so the cuDNN engine choice is captured eagerly and the histogram is complete), thennsys stats --report cuda_gpu_kern_sumwas grepped for conv kernels. The cuDNN engine choice is keyed on shape/dtype/alignment/groups and is independent of MLX graph capture, so graphs-off is representative of production.The discriminator is the launch count, not just the kernel name. The fast
conv1d_c1_k1_nhwcengine launches one kernel per conv op; the slowconvolve_common_enginelaunches one per channel (LFM2's 350M hit 286,720 launches, ~1024 per op). Every family measures at (conv layers x decode tokens) launches, i.e. one per op, which is direct positive proof of the fast engine rather than mere absence of the slow name.Per-family verdict
No family was CONFIRMED, so there are no before/after tok/s (nothing was adapted). Kernel counts are from the warm 24-token decode.
conv1d_c1_k1_nhwclaunchesconvolve_common_engineNotes on the NOT MEASURED rows:
kimi-vl-a3b-thinking-4bitis adeepseek_v3text backbone (MLA attention, no conv); it never exerciseskimi_linear.rs'sShortConv1d, so nsys shows no conv kernel at all. The actual Kimi-Linear architecture has no local checkpoint. ItsShortConv1d(conv1d -> silu, no bias) would fit the shared helper if a checkpoint appears.[B, C, L], convolves, transposes back), so it would need a small local adaptation rather than the shared helper. Documented, not adapted.Why LFM2 regressed but these do not
LFM2's slow dispatch is specific to its
conv_L_cache = 3(kernel width 3) at hidden 1024. cuDNN's engine heuristic selects the fast NHWC engine for the SSM families'conv_kernel = 4shapes and the slow generic per-channel engine only for LFM2's shape. The conv share of decode is 0.2-3.0% everywhere here, so even if a shape did tip over, the ceiling is small; adapting a refuted family would add the elementwise path for no benefit (and could be slower than the fast cuDNN kernel), so none is changed.PR #751 hypothesis correction (on the record)
PR #751's body speculated that the gemma-4-26b-a4b-it(-qat) (-10 to -14%) and glm4-flash-4bit (-14%) sweep drops were the same conv-dispatch pattern. Code inspection refutes that: neither is a conv architecture (
gemma4MoE andglm4_moe_lite; neither file callsconv1d), so their decode conv cannot fall into the grouped-conv engine. Those drops need separate attribution and are out of scope here. Similarly, mamba2-130m's -10.4% sweep delta is not conv dispatch (its conv is on the fast engine at 1.6% of decode); it is environmental sweep variance, as the issue anticipated.Test plan
cargo test --lib --features cuda models::conv_decode(5 passed: f32/bf16 kernel-3 and kernel-4 parity + silu/bias variant)cargo test --lib --features cuda models::lfm2(11 passed, unmodified)cargo clippy -p mlxcel --lib --tests --features cuda(clean; only pre-existing mlxcel-core C++ build notes)cargo fmt(no changes)cargo build --release --features cudathen nsys audit of all 10 candidate checkpoints on GB10Closes #752