Repository navigation
Conversation
|
Adding @am17an and @JohannesGaessler for review |
gentle reminder for the review |
|
Sorry, I hadn't prioritized looking at this PR for review because it is still marked as a draft. |
| if (up->op != GGML_OP_MUL_MAT || !ggml_cuda_mmq_fusion_supported(x_up->type) || y->type != GGML_TYPE_F32 || | ||
| ggml_get_op_params_i32(up, 1) != GGML_HINT_NONE || ggml_get_op_params_i32(gate, 1) != GGML_HINT_NONE) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!ggml_cuda_should_fuse_mul_mat(up, gate, glu)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I'm not sure this is logically correct. You can fuse matrices with the Hadamard hind, but whether you should is a different question. So the correct place to have this check would be in ggml_cuda_should_fuse_mul_mat_q.
There was a problem hiding this comment.
Moved the hint check into ggml_cuda_should_fuse_mul_mat_q. If I understand currently it does not trigger here: the hint is only set on the KV-cache rotation matmuls (F32 src0, not followed by a GLU), while the fused path only takes quantized gate/up weights.
| // Fused gate/up/GLU: src0 is the up weight, dst the GLU output. | ||
| const ggml_tensor * gate = fusion ? fusion->gate : nullptr; | ||
| if (fusion) { | ||
| GGML_ASSERT(gate && !ids && !fusion->x_bias && !fusion->gate_bias && !fusion->x_scale && !fusion->gate_scale); |
There was a problem hiding this comment.
| GGML_ASSERT(gate && !ids && !fusion->x_bias && !fusion->gate_bias && !fusion->x_scale && !fusion->gate_scale); | |
| // TODO MoE support | |
| GGML_ASSERT(gate && !ids && !fusion->x_bias && !fusion->gate_bias && !fusion->x_scale && !fusion->gate_scale); |
| // With fusion dst is the GLU output, not a MUL_MAT; the fused types always quantize src1 to Q8_1. | ||
| const ggml_prec prec_src1 = gate ? GGML_PREC_Q8 : ggml_cuda_mmq_get_prec_src1(src0, dst, cc); |
There was a problem hiding this comment.
| // With fusion dst is the GLU output, not a MUL_MAT; the fused types always quantize src1 to Q8_1. | |
| const ggml_prec prec_src1 = gate ? GGML_PREC_Q8 : ggml_cuda_mmq_get_prec_src1(src0, dst, cc); | |
| // With fusion dst is the GLU output, not a MUL_MAT; the fused types always quantize src1 to Q8_1. | |
| // TODO support for Q4 precision if both tensors allow it | |
| const ggml_prec prec_src1 = gate ? GGML_PREC_Q8 : ggml_cuda_mmq_get_prec_src1(src0, dst, cc); |
| if (!config.stream_k) { | ||
| mul_mat_q<type, J, fallback, prec_src1><<<block_nums_xy_tiling, block_dims, nbytes_shared, stream>>> | ||
| (args.x, args.y, args.ids_dst, args.expert_bounds, args.dst, nullptr, args.y_scale, | ||
| if (has_fusion || !config.stream_k) { |
There was a problem hiding this comment.
Since fusion is incompatible with stream-k, this PR should make it so that the expected efficiency of tiling vs. stream-k can be determined ahead of time and used in ggml_cuda_should_fuse_mul_mat_q.
| const int jt = has_fusion ? blockIdx.x : blockIdx.y; | ||
| const int it = has_fusion ? blockIdx.y : blockIdx.x; |
There was a problem hiding this comment.
Why are you swapping blockIdx.x and blockIdx.y?
There was a problem hiding this comment.
The fused kernel cannot use stream-k, so it runs the xy tiling with the column tiles innermost, the same order as the stream-k path (jt = tmp % ntx). With the row tiles innermost the concurrently running blocks each read a different weight tile, so the gate+up weights of a layer (100 MB for Qwen3.8-27B Q4_K vs 24 MB of L2 on spark) are streamed from VRAM once per column tile; with the column tiles innermost they share one weight tile and only y (~12 MB at 2048 tokens) has to stay in L2. Fused gate/up time per layer on spark, Qwen3.8-27B pp2048, row tiles innermost vs this PR: Q4_K 14.5 vs 10.1 ms, Q8_0 20.7 vs 9.1 ms, IQ4_XS 10.7 vs 8.7 ms.
| // Fused gate/up/GLU write-back: accumulators l and l + 2 hold the up and gate values of the same output row and column. | ||
| template<ggml_type type, int J, bool fallback> | ||
| static __device__ __forceinline__ void ggml_cuda_mmq_write_back_mma_glu( | ||
| const float * __restrict__ sum, const ggml_glu_op glu_op, const float glu_limit, | ||
| const int * __restrict__ ids_dst, float * __restrict__ dst, const int stride, const int i_max, const int j_max) { | ||
| #ifdef TURING_MMA_AVAILABLE | ||
| typedef tile<16, 8, int> tile_C; | ||
| static_assert(tile_C::I == 2*GGML_CUDA_MMQ_FUSION_ROWS, "each C tile must hold one chunk of up rows and one of gate rows"); | ||
|
|
||
| constexpr int rows_per_warp = ggml_cuda_mmq_get_rows_per_warp(type, J, fallback); | ||
| constexpr int ntx = rows_per_warp/tile_C::I; // Number of x minitiles per warp. | ||
|
|
||
| const int i0 = (threadIdx.y / ntx) * (ntx*tile_C::I); | ||
|
|
||
| #pragma unroll | ||
| for (int j0 = 0; j0 < J; j0 += ntx*tile_C::J) { | ||
| #pragma unroll | ||
| for (int n = 0; n < ntx; ++n) { | ||
| #pragma unroll | ||
| for (int l = 0; l < tile_C::ne/2; ++l) { | ||
| const int j = j0 + (threadIdx.y % ntx) * tile_C::J + tile_C::get_j(l); | ||
|
|
||
| if (j > j_max) { | ||
| continue; | ||
| } | ||
|
|
||
| const int i = (i0 + n*tile_C::I)/2 + tile_C::get_i(l); | ||
|
|
||
| if (fallback && i > i_max) { | ||
| continue; | ||
| } | ||
|
|
||
| const int sum_index = (j0/tile_C::J + n)*tile_C::ne + l; | ||
| dst[ids_dst[j]*stride + i] = ggml_cuda_op_glu_single(glu_op, sum[sum_index + 2], sum[sum_index], glu_limit); | ||
| } | ||
| } | ||
| } | ||
| #else | ||
| GGML_UNUSED_VARS(sum, glu_op, glu_limit, ids_dst, dst, stride, i_max, j_max); | ||
| NO_DEVICE_CODE; | ||
| #endif // TURING_MMA_AVAILABLE | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid adding a whole new function and extend the template for the existing one instead.
| if (up->op != GGML_OP_MUL_MAT || !ggml_cuda_mmq_fusion_supported(x_up->type) || y->type != GGML_TYPE_F32 || | ||
| ggml_get_op_params_i32(up, 1) != GGML_HINT_NONE || ggml_get_op_params_i32(gate, 1) != GGML_HINT_NONE) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think this is missing a check that up and gate have the same type.
There was a problem hiding this comment.
That is covered by ggml_cuda_should_fuse_mul_mat, called right below: it returns false unless up->src[0] and gate->src[0] have the same type, shape and strides, and ggml_cuda_mul_mat_q asserts the same. Happy to make it explicit here if you prefer.
|
|
||
| template <ggml_type type, bool fallback, ggml_prec prec_src1> | ||
| static void mul_mat_q_switch_fusion(ggml_backend_cuda_context & ctx, const mmq_args & args, cudaStream_t stream) { | ||
| if constexpr (ggml_cuda_mmq_fusion_supported(type)) { |
There was a problem hiding this comment.
| if constexpr (ggml_cuda_mmq_fusion_supported(type)) { | |
| if constexpr (!fallback && ggml_cuda_mmq_fusion_supported(type)) { |
In order to avoid compiling template specializations that make no sense to use. Add a check to disable fusion for cases with fallback == false and then you can also simplify the code a bit by using the assumption that nrows_x is a multiple of 128. Relevant PR: #29953 .
For prompt processing, run the gate and up matmuls of a dense FFN as one mul_mat_q launch and apply the GLU in the write-back. - The loaders take the gate weights as a second pointer and fill the weight tile with up and gate rows interleaved in chunks of 8, so each thread holds an up value and the matching gate value (rows r and r + 8 of an MMA tile). - The MMA loop is unchanged: same tile, warp layout, vec_dot and one sum[] per thread. The fused write-back computes the GLU from registers and writes half the rows. - The fused path uses the regular tiling (no stream-k) with the column tiles innermost so that neighbouring blocks share weight tiles. - Enabled for Q4_K, Q5_K, Q6_K, Q8_0 and IQ4_XS on NVIDIA Turing and newer.
0071207 to
b6e0f03
Compare
|
Addressed all the comments |
|
I ran this on Ada (RTX 4090, clocks locked to 2235 MHz, Qwen3.8-27B Q4_K, llama-bench -ngl 99 -r 5):
It helps on Ada too, though by less than on GB10 / RTX 5090. I also timed a single FFN block at the 27B shape (Q5_K, 5120 -> 17408 -> 5120, 512 tokens, median of 30 runs): 1441 us on master, 1414 us with this PR, and 1401 us with the down-projection GLU fusion in #30489. The fused launch is roughly at parity with the stock gate/up pair it replaces (~884 us vs ~880 us), so on Ada the gain comes mostly from removing the GLU kernel rather than from MMA-side savings; the down-side approach measures slightly faster per FFN. One interaction worth knowing about: With #30489 also in the tree, the two split coverage per FFN: this PR anchors the GLU first, so on the FFNs it accepts the down-side fusion does not run. On Ada the down-side fusion alone measures slightly faster (3030 vs 3023 t/s at pp512); both together 3040. Gating the fusion to cases where the plain dispatch would not pick stream-k might be worth checking on Ada. |
|
Thanks for the Ada numbers and the per-FFN breakdown. Scope. This PR is deliberately limited to the dense path and five types to keep it reviewable. The MUL_MAT_ID extension is already implemented locally on top of it (Qwen3.6-35B-A3B Q4_K_M, pp16K vs master: +10.2% on GB10, +6.1% on RTX 5090, perplexity bit-identical) and I plan to push it, and the remaining MMQ types, once the dense part is settled. Fused matmul vs the two stock matmuls. It depends on the type and the GPU, though. Per FFN layer, pp2048, 27B shape (5120 -> 17408), median time of the two stock MMQ launches vs the fused one, and the whole gate/up block:
Gating. Already in the PR since b6e0f03: #30489. Unsloth dynamic quants are a concrete case: some layers have different gate and up types, which this PR cannot interleave, so #30489 should take those layers while this PR takes the rest. I am measuring master / this PR / #30489 / both on Qwen3.8-27B UD-Q4_K_M on spark and will post the numbers here. |
|
Measured on spark with #30489 cherry-picked onto this PR. Qwen3.8-27B unsloth UD quants, pp16K, tok/s vs master on the same base:
UD-Q4_K_M has 30 of 64 FFN layers with different gate and up types. |
@JohannesGaessler : pinging here to move this up in your review stack |
Extends the gate/up + GLU fusion, which today only covers small batches (MMVQ/MMVF), to prompt-processing batch
sizes on the MMQ path.
Alternative to #28702 implementing @am17an's suggestion there: keep one accumulator and apply the GLU in the write-back instead of carrying a second accumulator tile for the gate as discussed here: #28702 (comment)
For prompt processing, the gate and up
MUL_MATs of a dense FFN and the followingGLUrun as a singlemul_mat_qlaunch. The small-batch MMVQ/MMVF fusion, other backends and the weight layout are unchanged.Implimentation
randr + 8of the MMA tile). Chunks of 8 rather than 16 also cover the one-tile-per-warp configs.sum[]are unchanged; the write-back applies the GLU from registers and writes 64 output rows per tile.Please note: I have a plan to validate and enable it for the other GGML also.
Performance
DGX Spark (GB10),
llama-bench -ngl 999 -fa on -p 16384 -n 0 -b 2048 -ub 2048 -ctk f16 -ctv f16 -r 5, tok/s:RTX 5090,
llama-bench -ngl 999 -fa on -p 16384 -n 0 -b 2048 -ub 2048 -ctk f16 -ctv f16 -r 5, tok/s:Correctness
test-backend-ops: 19 newMUL_MAT_VEC_FUSIONcases hit the MMQ path (129 rows with bounds checks at one and two MMA tiles per warp, 256 rows without, GEGLU / SWIGLU_OAI / SWIGLU_CLAMP, batched). Full suite passes on GB10; the MUL_MAT suites also pass on an RTX 5090.GGML_CUDA_DISABLE_FUSION=1for all five types, also with-ot ffn_gate=CPU.Requirements