[None][perf] Optimize Blackwell fused MHC half-MMA kernel - #16799
Conversation
Signed-off-by: MengmengSun <mengmengs@nvidia.com>
|
/bot run |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesThe fused TF32 pmap GEMM kernel adds packed Fused TF32 pmap GEMM
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant fused_tf32_pmap_gemm
participant global_coefficient_memory
participant cast_stage_pipeline
participant residual_output_TMA
fused_tf32_pmap_gemm->>global_coefficient_memory: Load valid post_mix and comb_mix rows
global_coefficient_memory-->>fused_tf32_pmap_gemm: Return masked float4 coefficients
fused_tf32_pmap_gemm->>cast_stage_pipeline: Synchronize cast stages and mix residuals
cast_stage_pipeline->>residual_output_TMA: Drain outstanding residual stores
residual_output_TMA-->>fused_tf32_pmap_gemm: Complete residual output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh (2)
441-458: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueVectorized
float4reinterpret onto plainfloat[4]locals relies on implicit compiler alignment.
*reinterpret_cast<float4*>(pm_u)/cm_u[j]requires 16-byte alignment, butpm_u/cm_uare declared as plainfloat[HC_MULT]/float[HC_MULT][HC_MULT]with no explicit alignment. This is a common working CUDA idiom for register-promoted locals, but is not guaranteed by the language. Consider declaring these asfloat4(oralignas(16) float[...]) to make the alignment explicit and avoid relying on compiler behavior if the surrounding code changes and these locals stop being register-promoted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh` around lines 441 - 458, Make the local buffers used by the float4 stores in the fused kernel explicitly 16-byte aligned, preferably by declaring pm_u, pm_l, cm_u, and cm_l with alignas(16) while preserving their existing float-array layout and indexing. Keep the loadCoefficientRow calls and subsequent consumers unchanged.
240-240: 🚀 Performance & Scalability | 🔵 TrivialReclaim now-dead
SMEM_POST_SIZE/SMEM_COMB_SIZEreservation.Since
post_mix/comb_mixare now loaded directly into registers vialoadCoefficientRow(lines 436-458),smem_post/smem_combare never referenced anywhere else in this kernel, yet the cursor still reservesSMEM_POST_SIZE + SMEM_COMB_SIZE(~5 KB per CTA for BLOCK_M=64/HC_MULT=4). Removing this reservation would free SMEM that could improve occupancy or enable other buffer growth — but check whether a companion host-side launcher computes the dynamic SMEM size from a shared formula that assumes this layout, since removing it here without that would need a matching update there.♻️ Possible reclaim (verify host-side smem-size formula first)
- cursor += SMEM_POST_SIZE + SMEM_COMB_SIZE; auto smem_rc = reinterpret_cast<nv_bfloat16*>(cursor); // [HC_MULT][BLOCK_M][BLOCK_K] bf16, single-buffered🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh` at line 240, Remove the unused SMEM_POST_SIZE and SMEM_COMB_SIZE reservation from the cursor advancement in the fused TF32 pmap GEMM kernel, since post_mix and comb_mix are loaded through loadCoefficientRow. Before finalizing, locate the companion host-side launcher’s dynamic shared-memory calculation and update its shared layout/size formula consistently so allocation matches the kernel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh`:
- Around line 441-458: Make the local buffers used by the float4 stores in the
fused kernel explicitly 16-byte aligned, preferably by declaring pm_u, pm_l,
cm_u, and cm_l with alignas(16) while preserving their existing float-array
layout and indexing. Keep the loadCoefficientRow calls and subsequent consumers
unchanged.
- Line 240: Remove the unused SMEM_POST_SIZE and SMEM_COMB_SIZE reservation from
the cursor advancement in the fused TF32 pmap GEMM kernel, since post_mix and
comb_mix are loaded through loadCoefficientRow. Before finalizing, locate the
companion host-side launcher’s dynamic shared-memory calculation and update its
shared layout/size formula consistently so allocation matches the kernel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec358972-5b52-435b-aac3-8f144996f774
📒 Files selected for processing (1)
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh
Signed-off-by: MengmengSun <mengmengs@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh (1)
240-240: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRelease the now-unused coefficient SMEM region.
SMEM_POST_SIZE + SMEM_COMB_SIZEstill reserves 5,120 bytes per CTA although this kernel no longer allocates or readssmem_post/smem_comb. Remove the cursor gap and reduce the corresponding dynamic-SMEM launch allocation.Proposed change
- constexpr uint32_t SMEM_POST_SIZE = BLOCK_M * HC_MULT * sizeof(float); - constexpr uint32_t SMEM_COMB_SIZE = BLOCK_M * HC_MULT * HC_MULT * sizeof(float); ... - // SMEM layout: [cd, B stages, res stages, x stages, post, comb, rc (HC_MULT slices)] + // SMEM layout: [cd, B stages, res stages, x stages, rc (HC_MULT slices)] ... - cursor += SMEM_POST_SIZE + SMEM_COMB_SIZE; auto smem_rc = reinterpret_cast<nv_bfloat16*>(cursor);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh` at line 240, Remove the SMEM cursor advance for the obsolete smem_post/smem_comb region near the affected kernel setup, and reduce the corresponding dynamic shared-memory launch allocation by SMEM_POST_SIZE + SMEM_COMB_SIZE. Ensure cursor layout and allocation remain consistent without reserving or accessing that unused region.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh`:
- Line 240: Remove the SMEM cursor advance for the obsolete smem_post/smem_comb
region near the affected kernel setup, and reduce the corresponding dynamic
shared-memory launch allocation by SMEM_POST_SIZE + SMEM_COMB_SIZE. Ensure
cursor layout and allocation remain consistent without reserving or accessing
that unused region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dbf530ed-eea6-4e0e-82a8-f909b89d45e8
📒 Files selected for processing (1)
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh
Signed-off-by: MengmengSun <mengmengs@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh (1)
202-233: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
smem_post/smem_combare now dead shared-memory allocations in this kernel.
SMEM_POST_SIZE/SMEM_COMB_SIZEare still reserved andsmem_post/smem_combstill declared here, but since the coefficient-loading prologue was removed (line 289) in favor of direct register loads viaload_row4(lines 439-459), these two pointers are never read anywhere else infused_tf32_pmap_gemm_rout_atomic_impl. This wastes 5,120 B of dynamic shared memory per CTA (1,024 B + 4,096 B) that could otherwise improve occupancy or be reallocated — directly relevant to this PR's stated optimization goal. It will also likely trigger unused-variable warnings.♻️ Proposed fix: remove the dead allocation
cursor += N_INPUT_STAGES * SMEM_X_PER_ISTG; - auto smem_post = reinterpret_cast<float*>(cursor); - cursor += SMEM_POST_SIZE; - auto smem_comb = reinterpret_cast<float*>(cursor); - cursor += SMEM_COMB_SIZE; auto smem_rc = reinterpret_cast<nv_bfloat16*>(cursor); // [HC_MULT][BLOCK_M][BLOCK_K] bf16, single-buffered🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh` around lines 202 - 233, Remove the unused SMEM_POST_SIZE and SMEM_COMB_SIZE constants and the smem_post/smem_comb declarations and cursor increments in fused_tf32_pmap_gemm_rout_atomic_impl. Keep the shared-memory layout and subsequent smem_rc allocation correctly contiguous after smem_x_stg, eliminating the 5,120-byte dead allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh`:
- Around line 202-233: Remove the unused SMEM_POST_SIZE and SMEM_COMB_SIZE
constants and the smem_post/smem_comb declarations and cursor increments in
fused_tf32_pmap_gemm_rout_atomic_impl. Keep the shared-memory layout and
subsequent smem_rc allocation correctly contiguous after smem_x_stg, eliminating
the 5,120-byte dead allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ece6c2e2-2fac-4f1b-9047-54e684028a0b
📒 Files selected for processing (1)
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuh
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #61505 [ run ] triggered by Bot. Commit: |
|
PR_Github #61505 [ run ] completed with state
|
Signed-off-by: MengmengSun <mengmengs@nvidia.com>
|
/bot run |
|
/bot run --disable-fail-fast |
|
PR_Github #61719 [ run ] triggered by Bot. Commit: |
|
PR_Github #61719 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61730 [ run ] triggered by Bot. Commit: |
|
PR_Github #61730 [ run ] completed with state |
The f32x2 mul/fma helpers are thin wrappers around PTX instructions and are not specific to mHC. Rename them after the PTX ops they wrap: mhc_f2ll -> bitcast_f32x2_to_u64 mhc_ll2f -> bitcast_u64_to_f32x2 mhc_mul2 -> mul_f32x2 mhc_fma2 -> fma_f32x2 mhc_fma2v -> fma_f32x2_vv Pure rename; no functional change. Signed-off-by: MengmengSun <mengmengs@nvidia.com>
|
/bot skip --comment 'Func name changing, has tested locally' |
|
PR_Github #61775 [ skip ] triggered by Bot. Commit: |
|
PR_Github #61775 [ skip ] completed with state |
Signed-off-by: MengmengSun <mengmengs@nvidia.com> Co-authored-by: MengmengSun <mengmengs@nvidia.com> (cherry picked from commit 08289b6)
…nnahz/dep-1083-port-flashinfer-stable-va-lifecycle-for-native-all-reduce * 'main' of https://github.com/NVIDIA/TensorRT-LLM: (54 commits) [NVIDIA#15673][fix] Enable CUDA core fast path for SM89/SM120/SM121 (NVIDIA#12705) [None][test] Adjust timeout cases in QA perf test (NVIDIA#16894) [https://nvbugs/6157892][fix] Mistral format refactor (NVIDIA#15123) [None][feat] Add kimi_k2/glm_5 grouped routing and fused router to bench_moe (NVIDIA#16830) [https://nvbugs/6501376][fix] Test-only fix — drop the `if hidden_size % 2 != 0: with pytest.raises(...)`… (NVIDIA#16844) [TRTLLM-13642][feat] Add perf sanity tests for Llama-3.1-8B and Gemma-3-1B and verify cache transceiver V2 support (NVIDIA#16355) [https://nvbugs/6433376][fix] Update the Dense test to mirror the MoE sibling — assert `bfloat16` under… (NVIDIA#16203) [None][fix] Resolve NVFP4 mixed-precision base layers for the DSpark draft (NVIDIA#16831) [https://nvbugs/6479324][test] Remove waiver for fixed qwen3_5_4b_fp8_stress disaggregated stress test (NVIDIA#16878) [https://nvbugs/6507109][infra] Split slow DGX B300 attention unit tests (NVIDIA#16838) [None][infra] Waive 21 failed cases for main in post-merge 2862 (NVIDIA#16882) [None][perf] prepare_inputs: avoid O(seq_len) get_tokens(0) marshalling on the host (NVIDIA#16791) [None][perf] Optimize Blackwell fused MHC half-MMA kernel (NVIDIA#16799) [None][infra] Auto-update test durations from OpenSearch (last 7 days) [None][perf] Skip DeepGEMM clean_logits in DSA indexer prefill on custom top-k path (NVIDIA#16789) [None][feat] Support DeepSeek-V4 in layer_wise_benchmarks (NVIDIA#16774) [https://nvbugs/6465993][fix] use attention cache dtype for disaggregated transfer (NVIDIA#16505) [https://nvbugs/6463822][fix] Fix LTX2 CUDA graph test leak issue (NVIDIA#16775) [https://nvbugs/5948435][chore] Unwaive DeepSeekV3Lite test_nvfp4_4gpus CUTLASS ep4 fp8kv on RTXPro6000D (NVIDIA#16621) [TRTLLM-14417][fix] Exclude ADP/cuda-graph dummy requests from speculative-decode acceptance stats (NVIDIA#16571) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
…nnahz/dep-1082-shared-mnnvl-moe-lifecycle * 'main' of https://github.com/NVIDIA/TensorRT-LLM: (142 commits) [NVIDIA#15673][fix] Enable CUDA core fast path for SM89/SM120/SM121 (NVIDIA#12705) [None][test] Adjust timeout cases in QA perf test (NVIDIA#16894) [https://nvbugs/6157892][fix] Mistral format refactor (NVIDIA#15123) [None][feat] Add kimi_k2/glm_5 grouped routing and fused router to bench_moe (NVIDIA#16830) [https://nvbugs/6501376][fix] Test-only fix — drop the `if hidden_size % 2 != 0: with pytest.raises(...)`… (NVIDIA#16844) [TRTLLM-13642][feat] Add perf sanity tests for Llama-3.1-8B and Gemma-3-1B and verify cache transceiver V2 support (NVIDIA#16355) [https://nvbugs/6433376][fix] Update the Dense test to mirror the MoE sibling — assert `bfloat16` under… (NVIDIA#16203) [None][fix] Resolve NVFP4 mixed-precision base layers for the DSpark draft (NVIDIA#16831) [https://nvbugs/6479324][test] Remove waiver for fixed qwen3_5_4b_fp8_stress disaggregated stress test (NVIDIA#16878) [https://nvbugs/6507109][infra] Split slow DGX B300 attention unit tests (NVIDIA#16838) [None][infra] Waive 21 failed cases for main in post-merge 2862 (NVIDIA#16882) [None][perf] prepare_inputs: avoid O(seq_len) get_tokens(0) marshalling on the host (NVIDIA#16791) [None][perf] Optimize Blackwell fused MHC half-MMA kernel (NVIDIA#16799) [None][infra] Auto-update test durations from OpenSearch (last 7 days) [None][perf] Skip DeepGEMM clean_logits in DSA indexer prefill on custom top-k path (NVIDIA#16789) [None][feat] Support DeepSeek-V4 in layer_wise_benchmarks (NVIDIA#16774) [https://nvbugs/6465993][fix] use attention cache dtype for disaggregated transfer (NVIDIA#16505) [https://nvbugs/6463822][fix] Fix LTX2 CUDA graph test leak issue (NVIDIA#16775) [https://nvbugs/5948435][chore] Unwaive DeepSeekV3Lite test_nvfp4_4gpus CUTLASS ep4 fp8kv on RTXPro6000D (NVIDIA#16621) [TRTLLM-14417][fix] Exclude ADP/cuda-graph dummy requests from speculative-decode acceptance stats (NVIDIA#16571) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Dev Engineer Review
Updated
cpp/tensorrt_llm/kernels/mhcKernels/fused_tf32_pmap_gemm.cuhto optimize the Blackwell fused MHC pmap/cast pipeline used by the two-kernelfused_half_mmapath via:float2arithmetic helpers innamespace fused_mhc(with an arch-gated PTX path when available and scalar fallbacks otherwise), including:bitcast_f32x2_to_u64,bitcast_u64_to_f32x2mul_f32x2,fma_f32x2,fma_f32x2_vvpost_mix/comb_mixrows into SMEM; instead it loads the needed rows directly from global memory into per-thread registers using validity-maskedfloat4-style loads (deterministic zeroing for out-of-range).float2(sqr2_u/sqr2_l), with later reduction derived from the packed components.float2sequence:smem_rc) by draining in-flight residual TMA stores withtma_store_wait<0>()whenht > 0,float2FMA.empty_input[i_stage]->arrive()outside the inner restructuring and draining residual TMA once viatma_store_wait<0>(); computing warp-reduction inputs forsqr_sumfromsqr2_u.x + sqr2_u.y/sqr2_l.x + sqr2_l.y.Scope preserved: kernel signatures, tactics, launch configuration, shared-memory allocation, APIs/dependencies, and build configuration remain unchanged.
Correctness/performance review focus:
float2bitcasts and PTXmul.f32x2/fma.f32x2behavior vs scalar fallbacks.tma_store_wait<0>()occurs before reusingsmem_rcto avoid residual race regressions.Follow-up change: renamed packed FP32x2 PTX helper wrapper names to generic forms (
mul_f32x2,fma_f32x2, and related bitcast helpers), described as a functional no-op.Expected impact (per objective): on NVIDIA B300, half-MMA workloads for
M=32–16384improve by ~4.24%–19.86%, while half-FMA forM=1–16stays within about -0.25% to +0.77% of baseline.QA Engineer Review
No test changes.
Description
This PR optimizes the two-kernel fused_half_mma path used by fused MHC on Blackwell GPUs.
This change improves that kernel by:
mul.rn.ftz.f32x2andfma.rn.ftz.f32x2on SM100-family Blackwell GPUs, including SM103/B300. The scalar implementation remains available as the fallback for other architectures.float4register loads. Residual output stores are issued through TMA afterthe tile compute.
The existing kernel signature, tactic definitions, launch configuration, and dynamic shared-memory allocation are preserved. There are no API, dependency, or build-system changes.
Test Coverage
mhc_fused_hcfull-op through TensorRT-LLM'sMhcFusedHcRunner, including BigFuseH=7168,n=4,M=1,2,4,...,16384e96c3309f3f3bae77e6b6a4ec95fa046fd40eced6e214bd94f06a2a23108a3e4bbfd0c7769746f1bMhcFusedHcRunner.get_valid_tactics(inputs, profile=None)for each build;all production FMA and MMA backends are measured
(baseline_us - optimized_us) / baseline_us x 100In this paired reference, the unchanged FMA-selected region (
M=1..16) stays within -0.25% to +0.77%. The affected production-best MMA region(M=32..16384) improves every shape by 4.24% to 19.86%, with a 9.77% median and 10.98% mean improvement.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.