Repository navigation
Conversation
…k-scale DeepGEMM kernel On the GroupedWithOffset path of fp8_gemm_kernel with BLOCK_N a multiple of 64, read the B scales for each K block from global memory instead of staging them in shared memory behind a barrier, and write each math warp group's rows through its own swizzled half of smem_d with coalesced stores instead of a TMA store. This removes the 8-way bank conflicts of the STSM write-back and the two per-tile barriers that kept the two math warp groups in lockstep. Output is bitwise unchanged; other GEMM types, other BLOCK_N and the swap-AB kernel keep the original code. Signed-off-by: Milo <7bit2333@gmail.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe FP8 GEMM kernel adds a warp-group-local epilogue for qualifying grouped-with-offset tiles. It loads B scales directly from global memory on that path and retains the existing shared-memory output epilogues for other configurations. ChangesFP8 GEMM epilogue
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant MathWarpGroup
participant SharedMemory
participant GlobalMemory
MathWarpGroup->>SharedMemory: Stage bfloat16 output in XOR-swizzled layout
MathWarpGroup->>GlobalMemory: Write bounded 16-byte output chunks
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review. The PR is mergeable subject to normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Dev Engineer Review
The kernel adds a compile-time epilogue for
GroupedWithOffsetwhenBLOCK_Ncontains whole groups of eight 16-byte chunks. That path reads B scales from global memory and uses XOR-swizzled shared-memory staging, warp-group barriers, and coalesced global stores. It checks M boundaries and N tails. Other configurations retain the existing epilogue.The change affects scale loading and output synchronization for the selected path. Validate scale indexing, partial M/N tiles, and warp-group synchronization. The supplied summary reports bitwise-identical outputs and H20 performance gains, but the test results are not independently available here.
QA Engineer Review
No test code changed.
tests/integration/test_lists/waives.txtadds two SKIP entries:test_flashinfer_gdn_decode.pyand onetest_mm_encoder_standalone.pyparameterization. The diff does not establish changes to CI or manual-QA test membership. The stated kernel validation and compute-sanitizer checks are author-reported; TensorRT-LLM CI was not run locally. Coverage verdict: needs follow-up, because this PR adds no test coverage for the changed kernel path.Per-File QA Perspective
cpp/include/tensorrt_llm/deep_gemm/fp8_gemm_impl.cuh: Verify selected grouped-with-offset cases, scale loading, boundary rows, N tails, and output correctness. Confirm unaffected paths continue to use the original epilogue.tests/integration/test_lists/waives.txt: Two unrelated tests are newly waived with bug references. These entries change waiver behavior; they do not add coverage for the kernel changes.Description
On Hopper, the CUTLASS fused MoE with FP8 block scales runs both expert GEMMs through
fp8_grouped_gemm_run→grouped_gemm_dispatch, which uses the JIT-compiled DeepGEMMfp8_gemm_kernel(GemmType::GroupedWithOffset) onceexpected_m(the token count) reaches 32, or 64 on 78-SM GPUs. For MoE shapes it picks 128×128 tiles, so two math warp groups share each output tile. Much of this kernel's time goes to a fixed cost per output tile, which weighs most when K is short. On H20, the FC2 GEMM of a Qwen3-Next-style layer (N=2560, K=640, 128 local experts, ~1175 rows per expert) reaches 66% of FP8 peak (valid rows only). Two things cost the most:smem_dis a plain row-major tile (256-byte rows forBLOCK_N= 128) because the TMA store usesSWIZZLE_NONE. Each 8×8 STSM matrix covers the same 16-byte column of 8 consecutive rows, so every STSM is an 8-way bank conflict (about 9% of that GEMM's time).smem_d. They keep the two math warp groups in lockstep (removing the tile-start barrier with the B-scale staging saves about 7%, removing the store wait about 5%).This PR changes only the non-swap-AB kernel, and only on the
GroupedWithOffsetpath withBLOCK_Na multiple of 64 (64 or 128 of the 16..128 candidates):__ldgfor each K block before waiting for that block's TMA data and are used after its WGMMA, so the load hides behind the MMA. They are no longer staged in shared memory, and the tile-start barrier is gone.smem_d, with 16-byte chunks XOR-swizzled byrow % 8(conflict-free), syncs only its own 128 threads, and copies the rows out with coalesced 16-byte global stores, masking rows past the expert (m_boundary) and columns past N. A second sync of the same 128 threads protects the rows from the next tile's STSM. There is no TMA store, no store wait and no synchronization between the two math warp groups.Normal,StridedBatched, otherBLOCK_Nvalues (e.g. 120) and the swap-AB kernel are unchanged. Outputs are bitwise identical.The same file is vendored in FlashInfer, where the change is proposed in flashinfer-ai/flashinfer#5664 with the full data. Kernel time on H20 (78 SMs), CUDA 13.3, 1175 rows per expert, tile config chosen by
get_best_gemm_config(128×128, 5 stages here, also on 132 SMs), L2 flushed before each launch, median of 5 interleaved rounds:The kernel compiled from this PR's header produces the same SASS as the FlashInfer version these numbers were measured with. End to end, the change raised serving throughput of Qwen3.8-Flash-Next-FP8 on 4×H20 (vLLM with FlashInfer's copy of this kernel) by 2.8%.
JIT cache note: with the default NVRTC path compiled kernels are not written to disk, so they pick up the change. With
TRTLLM_DG_JIT_USE_NVCC=1orTRTLLM_DG_JIT_DUMP_CUBIN=1, cubins under$TRTLLM_DG_CACHE_DIR/cacheare named only by shape and tile config and would keep being loaded until that directory is cleared.Test Coverage
tests/unittest/_torch/moe/test_moe_module.py::test_configurable_moe_single_gpuwithFP8_BLOCK_SCALESon the CUTLASS backend (Hopper) runs this kernel. I could not run the TensorRT-LLM test suite locally (no TensorRT-LLM build on this machine); please trigger CI.deep_gemm::jit::Compilerbuilds it and host code mirroringrunGemm: 392 cases overBLOCK_M64/128 ×BLOCK_N64/128 (plus 120/112/96/16, which keep the original code), N with and without a partial last block (2560, 1280, 2112, 2080, 1000 and long-K shapes up to K = 7168), 1 to 128 experts, and per-expert row counts including 0, 1, tile-boundary values ±1, runs of empty experts and partial last tiles, with NaN in the stale rows the tiles read past each expert. Every valid output element is bitwise equal to an exact reference and to the original kernel, and no row past the valid ones is written.memcheck,racecheck,synccheckandinitcheck: no errors or hazards; 1000 back-to-back launches give identical outputs.cutlass_fused_moe(same kernel), outputs are bitwise identical before and after the change on 16 (config, batch) cases, and a batch large enough to use this kernel matches the same tokens run 16 at a time on the swap-AB kernel bit for bit.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.