Repository navigation
perf(moe): share gather indices across SwitchGLU projections - #2137
Merged
Merged
Conversation
inureyes
added a commit
that referenced
this pull request
Oct 6, 2026
3 of 5 tasks
Every gather_qmm / gather_mm call with no lhs_indices makes MLX synthesize an Arange, a Reshape and a Broadcast, and every non-uint32 expert-id tensor gets an AsType, so a SwitchGLU block built those nodes once per projection on every decode step. SwitchGLU now builds the uint32 expert ids and gate/up's lhs_indices once (prepare_gather_indices) and passes them through the new SwitchLinear::forward_indexed; the sorted prefill path shares one lhs across all three projections. The input expand_dims pair and the expert-scale expand_dims pairs collapse to one expand_dims_multi node. A new arange_u32 bridge function builds the lhs exactly as MLX's indices_or_default does, so no cast node is added. The arrays passed are the values MLX would have built, so the kernels see identical inputs. On GB10 (CUDA) the decode graph drops 4 nodes and 7 edges per MoE layer: granite-4.0-h-tiny-4bit 3419 -> 3259 nodes (9057 -> 8777 edges), qwen3-30b-a3b-4bit with MLXCEL_FUSED_MOE=0 2895 -> 2703 nodes. Greedy --temp 0 output is byte-identical on granite-4.0-h-350m/tiny, qwen3-30b-a3b and qwen3.5-35b-a3b. New bit-exactness tests pin the prepared indices and the block output against the per-call form. Refs #1713
inureyes
force-pushed
the
refactor/issue-1713-moe-helper-op-count
branch
from
October 6, 2026 09:49
98d80bf to
42b7c26
Compare
This was referenced Oct 6, 2026
inureyes
added a commit
that referenced
this pull request
Oct 8, 2026
ROCm verification of the unified batch-native engine (epic #2166 Phase 7) on gfx1151. Every test #2192 names ran and passed, including the ignored 20 GiB paged pool test; each ROCm behavior was traced to its post-epic location on the engine path; CLI and server parity arms are identical, and dense versus paged with the HIP paged v2 kernel is greedy-identical with 0 decided-position mismatches; 10 of 11 decode cells are within the 1 percent threshold of the pre-epic CxxGenerator. Adds the results page and its data, the ROCm parity definition in ADR 0007 and docs/CONTINUOUS_BATCHING.md, and corrects one stale comment in src/models/llama3.rs. Regressions are filed as #2239 (Gemma 3 pp2048 decode -1.19 percent) and #2241 (MoE sorted prefill lost right_sorted since #2137). Verified locally with make verify-rocm on gfx1151 at main 1b4e365: 12,194 passed, 0 failed, 403 ignored. Closes #2192
inureyes
added a commit
that referenced
this pull request
Oct 10, 2026
## Summary - The sorted prefill path of `SwitchGLU::forward` lost `right_sorted` when #2137 shared one explicit identity `lhs_indices` across gate, up and down: upstream `gather_qmm` / `gather_mm` build the primitive with `right_sorted = sorted_indices && !lhs_indices`, and every backend's sorted MoE prefill kernel keys on that flag, so affine MoE prefill fell back to per-row kernels, 1.7x to 4.8x slower on gfx1151. ## What changed - `src/models/switch_layers.rs`: the sorted path now shares only the `uint32` expert ids and leaves `lhs_indices` to MLX, whose default is the same arange; the unsorted (decode) sharing of #2137 is unchanged, so its decode-graph node cut stays. `SwitchLinear::forward_indexed` documents the constraint and debug-asserts it. - `src/lib/mlxcel-core`: new `gather_sorted_flags` bridge accessor walks an unevaluated graph and reports the `left_sorted` / `right_sorted` state of every `GatherQMM` / `GatherMM` node. - `docs/environment-variables.md`: the `MLX_ROCM_GATHER_QMV_EXPERT_BATCHED` row records that the op sets `right_sorted` only when the caller passes `sorted_indices` and no `lhs_indices`. - Adds the measurement data and driver under `docs/benchmark_results/data/rocm-moe-prefill-right-sorted-gfx1151-2026-10-09/`. ## Benchmarks (gfx1151, pp512, medians of 3 guarded runs, arms alternated) - granite-4.0-h-tiny-4bit prefill 606.91 to 918.41 tok/s (2026-10-05 page: 918.59) - Mixtral-8x7B-Instruct-v0.1-4bit 25.33 to 122.21 tok/s (page: 125.63) - Qwen3-30B-A3B-4bit 291.26 to 318.36 tok/s - gpt-oss-20b-MXFP4-Q4 465.74 to 500.10 tok/s (unaffected path, GptOssSwitchGLU); decode unchanged on all four models - rocprofv3: `gather_qmv_expert_batched_kernel` back at 240 launches (42.7 percent of kernel time) where the pre-fix build had none ## Reproduction - `models::switch_layers::gather_index_tests::sorted_prefill_keeps_right_sorted_on_every_projection` pins `right_sorted` on all three projections for quantized and regular experts without running a kernel. On the previous `main` it fails with "projection 0 lost right_sorted: the sorted path passed an explicit lhs_indices"; with the fix it passes (1 passed, 0 failed, 9071 filtered out, both runs on gfx1151). Metal and CUDA cannot be measured on this Linux host; the flags are computed in the op constructor, backend-independent, and the test pins them for every backend. Verified locally with `make verify-rocm` on gfx1151 at d3faa75: 12,199 passed, 0 failed, 403 ignored. Closes #2241
inureyes
added a commit
that referenced
this pull request
Oct 14, 2026
…fast paths (#1859) (#2310) ## Summary Closes #1859. Attributes the single-token (`n_tokens = 1`) decode throughput gap between `mlxcel` and `mlx-lm` / `mlx-vlm` on `deepseek-v4-flash-4bit`, `inkling-small-mlx-4bit`, and `inkling-small-nvfp4`, and lands the two concrete MoE fast paths identified by the attribution. ### Step 2 Attribution Record (`docs/benchmark_results/moe-m1-gather-attribution-2026-10-14.md`) - **Committed first in `9072e2ad`** prior to any edits to `src/models/switch_layers.rs`, `src/models/deepseek_v4_moe.rs`, or `src/models/inkling/mlp.rs`. - **Metal `GatherQMM::eval_gpu` (`overlays/metal/mlx/backend/metal/quantized.cpp:2060-2088`)**: Checks `M < get_qmv_batch_limit(K, N, d)`. Since `get_qmv_batch_limit >= 6` on all Apple GPUs (`12..33` on Max/Ultra/M5) and `M = 1`, `GatherQMM` unconditionally returns through `gather_qmv` (`affine_gather_qmv_fast_bfloat16_gs_64_b_4` or `fp_gather_qmv_fast_bfloat16_nvfp4`). Commit `8ad62d4b` (`#1772`, `bm = 32` inside `gather_qmm`) is unreachable at `M = 1`. - **CUDA `GatherQMM::eval_gpu` (`overlays/cuda/mlx/backend/cuda/quantized/quantized.cpp:526-547`)**: Checks `if (can_use_qmm_sm80) { if (can_use_qmv && (M * B < 8)) call_qmv(); else call_qmm_sm80(); }`. Unlike `top_k = 8` models (`M * B = 8 >= 8`), `deepseek-v4-flash-4bit`, `inkling-small-mlx-4bit`, and `inkling-small-nvfp4` all use `top_k = 6`, so `M * B = 1 * 6 = 6 < 8` is **true** at single-token decode. Furthermore, `supports_qmm_sm80` is false for `nvfp4` (`inkling-small-nvfp4`). All three models therefore already dispatch `M = 1, B = 6` expert projections to `mlx::core::cu::gather_qmv_kernel`, never `qmm_sm80` (`tile_m = 16`). - **ROCm `GatherQMM::eval_gpu` (`overlays/rocm/mlx/backend/rocm/quantized/qmm.hip:6506-6726`)**: Dispatches `M = 1, B = 6` to `gather_qmv_wide_kernel` (`affine` `bf16`) or `gather_qmv_kernel` (`nvfp4`). - **Reconciled `forward_fused_kernel` inventory**: 17 model families wired to `SwitchGLU::forward_fused_kernel` + 2 family-local (`qwen3_next`, `gemma4`) = 19 families on `096152c6` (growing to 20 with `inkling` in this PR). - **`deepseek-v4-flash-4bit` decode gap breakdown**: Over 85% of the gap is non-MoE—specifically the 2x per-layer (`86x/token`) unfused 20-iteration float32 `HyperConnection` Sinkhorn loops (`src/models/deepseek_v4_hyper.rs:38-42, 115-200`, ~290 graph nodes/layer = ~12,470 nodes/token vs. upstream's fused `_hc_sinkhorn_collapse_kernel`), plus MLA KV compression and Indexer. Within the MoE block, `LimitedSwitchGlu::forward` missed `#1713` / PR `#2137`, emitting 6 redundant index-synthesis graph nodes per layer (`258` nodes/token across 43 layers). - **`inkling-small-mlx-4bit` and `inkling-small-nvfp4` decode gap breakdown**: `InklingSparseMoe::forward` (`src/models/inkling/mlp.rs:217-223`) never called `SwitchGLU::forward_fused_kernel`, leaving `inkling-small-mlx-4bit` (`affine` 4-bit gs64, `gate_scale = None, out_scale = None`) on the 6-launch / 15-node `gather_qmv` + `moe_weighted_sum` path instead of the 2-launch / 5-node fused kernel path. Non-MoE contributors are the 4x per-layer f32 short convolutions, unfused banded relative-position bias, 24-op `route_weights`, and on GB10 CUDA for `inkling-small-nvfp4`, `MLX_MAX_MB_PER_BUFFER = 25` (`data_size()` element count) forcing a CUDA graph commit per `nvfp4` `gather_qmm`. ### Step 3 Implementation (`f7733f8b`) - **`src/models/inkling/mlp.rs`**: Wire `InklingSparseMoe::forward` at single-token decode (`flat.shape()[0] == 1 && self.gate_scale.is_none() && self.out_scale.is_none() && fused_moe_enabled()`) to `SwitchGLU::forward_fused_kernel`, saving 4 GPU kernel launches and 10 graph nodes per sparse MoE layer on `inkling-small-mlx-4bit` while keeping `inkling-small-nvfp4` (`gate_scale = Some, out_scale = Some`) on `forward_with_expert_scales`. - **`src/models/deepseek_v4_moe.rs`**: Update `LimitedSwitchGlu::forward` to use `expand_dims_multi(x, &[-3, -2])`, `prepare_gather_indices`, and `SwitchLinear::forward_indexed`, eliminating 6 redundant index-synthesis graph nodes per layer (`258` nodes/token across 43 layers). - **`src/models/switch_layers.rs`**: Update `Used by:` doc comments on `fused_moe_max_dff_from`, `SwitchLinear::forward`, `SwitchLinear::forward_indexed`, `SwitchGLU::forward_fused_kernel`, and `prepare_gather_indices`. ## Verification - `cargo fmt --all -- --check` - `MLXCEL_DEVICE=cpu cargo test --profile test-fast --lib models::deepseek_v4::tests::limited_switch_glu -- --test-threads=1` (1 passed) - `MLXCEL_DEVICE=cpu cargo test --profile test-fast --lib models::inkling::mlp::tests -- --test-threads=1` (3 passed) - `MLXCEL_DEVICE=cpu cargo test --profile test-fast --lib models::inkling::tests -- --test-threads=1` (passed) - `MLXCEL_DEVICE=cpu cargo test --profile test-fast --lib models::switch_layers::gather_index_tests -- --test-threads=1` (5 passed) - `cargo clippy --profile test-fast -p mlxcel --lib --tests -- -D warnings` (0 warnings)
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
Each
gather_qmm/gather_mmcall with nolhs_indicesmakes MLX add anArange+Reshape+Broadcastto the graph, and a non-uint32expert-id tensor adds anAsType.SwitchGLUpaid for this once per projection on every step. It now builds those index arrays once per block and shares them across gate, up and down. Arithmetic is unchanged because the arrays it passes hold the same values MLX would build.prepare_gather_indices+SwitchLinear::forward_indexed: gate/up share onelhs_indicesarray (unsorted path), all three projections share one (sorted prefill path), and theuint32ids are cast once. Down keeps MLX's default lhs in the unsorted path because its batch shape is[n, k].expand_dimspairs on the input and on the Inkling expert-scale sidecars become oneexpand_dims_multinode each.arange_u32bridge function builds the lhs exactly as MLX'sindices_or_defaultdoes (arange(stop, uint32)), so no cast node is added.SwitchLinear::forward(used directly by 15 families) is unchanged and delegates withlhs = None.Evidence (GB10, CUDA,
MLXCEL_EXPORT_DECODE_DOT, first decode step)MLXCEL_FUSED_MOE=0(48 layers)Per MoE layer: Arange, Broadcast, ExpandDims and Reshape each drop by 1, and no other op kind changes. Granite's remaining Full, Slice and AsType nodes come from the SSM and attention paths, which this PR does not change.
Greedy
mlxcel generate --temp 0 -n 64output is byte-identical between the base and new binaries on granite-4.0-h-350m, granite-4.0-h-tiny, qwen3-30b-a3b (fused on and off) and qwen3.5-35b-a3b (fused on and off). A second base run matched the first, so the comparison is deterministic. The prompt runs prefill through the sorted path (tokens x top_k >= 64), and decode runs through the unsorted path.Throughput is informational only. With pp512/tg128, n=3 interleaved rounds per arm, and a null arm (a byte-identical copy of the base binary), the null arm moved up to -34% from base inside a single round. All base/new medians sit inside that spread (decode tok/s, base / new / null: granite-tiny 100.3 / 100.1 / 100.4, qwen3-30b-a3b default 80.1 / 75.1 / 75.4, qwen3.5-35b-a3b 59.2 / 62.3 / 60.1). On qwen3-30b-a3b the new binary matches the null copy, and decode there runs the fused kernel, which this PR does not touch. The
forwardms/token medians are also flat (granite-tiny 1.44 vs 1.40, null 1.40). The 4.7% node cut predicts about 0.05 ms, which is below this host's noise. I make no throughput claim.Test plan
models::switch_layerson CUDA (36 tests, includingmxfp_testsand 4 newgather_index_teststhat pin bit-exactness against the per-call form in decode, small-prefill and sorted-prefill shapes with int32 and uint32 ids)cargo clippy --release --lib --tests --features cuda -- -D warnings,cargo fmt --checkNot verified on this host
benchmarks/pylm_m5max_2026-09-06.csvand the Metalforwardms/token: these need Apple Silicon.mxfp_tests([nightly-verify] main is red #1599): this PR does not touch mxfp-specific dispatch, and those tests pass on CUDA.Closes #1713