[TRTLLM-15316][feat] sm107 gemm + quant - #17485
Conversation
|
/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)
WalkthroughThe change broadens SM100-family CUDA build and dispatch handling, including SM107. It adds configurable MXFP8 scale-vector sizes and replication, plus optional R128c4 output while preserving the legacy packed layout by default. ChangesBlackwell architecture support
MXFP8 scale-factor layouts
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The current change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant TorchCaller
participant fp8_quantize_1x128_packed_ue8m0
participant invokeMxFP8Quantization
participant MXFP8Kernel
TorchCaller->>fp8_quantize_1x128_packed_ue8m0: pass use_r128c4_layout
fp8_quantize_1x128_packed_ue8m0->>invokeMxFP8Quantization: select scale-vector configuration
invokeMxFP8Quantization->>MXFP8Kernel: launch MXFP8 quantization
MXFP8Kernel-->>fp8_quantize_1x128_packed_ue8m0: return FP8 values and selected scale layout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@cpp/tensorrt_llm/kernels/cutlass_kernels/python/generate_kernels.py`:
- Around line 1008-1011: Update the SM100 branch in the architecture-generation
loop to pass the has_arch predicate to generate_sm100_operations instead of the
integer sm, preserving the existing enabled-architecture check and break
behavior.
In `@cpp/tensorrt_llm/kernels/quantization.cu`:
- Around line 418-423: Add the missing explicit instantiation for
invokeMxFP8Quantization<__nv_bfloat16, 128, 128> alongside the existing BF16
specializations, ensuring the header-exposed default resolves correctly;
alternatively, remove the unsupported default if that specialization is
intentionally unavailable.
In `@cpp/tensorrt_llm/thop/fp8Quantize.cpp`:
- Line 274: Update the test calls to fp8_quantize_1x128_packed_ue8m0 at both
referenced cases so they explicitly pass use_r128c4_layout=False, preserving the
existing assertions for the legacy packed layout.
In `@tensorrt_llm/_torch/custom_ops/cpp_custom_ops.py`:
- Around line 778-788: Add the return type annotation `tuple[torch.Tensor,
torch.Tensor]` to the fake operator function signature, preserving its existing
parameters and return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a0aa5a8-72ee-4a7a-a6c0-3eaba60748e4
📒 Files selected for processing (14)
cpp/tensorrt_llm/kernels/cutlass_kernels/CMakeLists.txtcpp/tensorrt_llm/kernels/cutlass_kernels/cutlass_heuristic.cppcpp/tensorrt_llm/kernels/cutlass_kernels/cutlass_preprocessors.cppcpp/tensorrt_llm/kernels/cutlass_kernels/fp4_gemm/fp4_gemm_template.hcpp/tensorrt_llm/kernels/cutlass_kernels/fp8_rowwise_gemm/fp8_rowwise_gemm_template.hcpp/tensorrt_llm/kernels/cutlass_kernels/fpA_intB_gemm/fpA_intB_gemm_template.hcpp/tensorrt_llm/kernels/cutlass_kernels/python/generate_kernels.pycpp/tensorrt_llm/kernels/quantization.cucpp/tensorrt_llm/kernels/quantization.cuhcpp/tensorrt_llm/kernels/quantization.hcpp/tensorrt_llm/thop/fp8BlockScalingGemm.cppcpp/tensorrt_llm/thop/fp8Quantize.cpptensorrt_llm/_torch/custom_ops/cpp_custom_ops.pytensorrt_llm/_torch/custom_ops/torch_custom_ops.py
08061ec to
14cbc87
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
• ### [P1] Preserve the existing default output contract This op previously accepted a single argument, returned the legacy strided int32 [m, Defaulting use_r128c4_layout to True silently changes the output dtype, shape, and Please keep use_r128c4_layout=False as the default and let the future Rubin consumer ——— [P1] Add target-architecture coverage for the newly enabled SM107 pathsThe PR description says the existing GEMM suites cover the dispatch changes, but the The relevant FP4 linear and MoE GEMM tests also remain restricted to SM100/SM103: TensorRT-LLM/tests/unittest/_torch/thop/parallel/test_fp4_linear.py Lines 100 to 112 in 14cbc87 The new quantization tests validate the producer layout, but they do not validate Before merging, please provide:
B200 and H100 stages cannot validate the newly enabled SM107 runtime paths. ——— [P2] Make the SM107 TRT-LLM Gen compatibility rule explicitThis PR allows SM107 to enter TrtllmGenGemmRunner, but isSMCompatible(107, kernelSM) The current generated metadata happens to contain only Sm100f candidates for this Please add an explicit SM107 rule that accepts only Sm100f kernels before enabling |
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
Approving on behalf of trt-llm-runtime-devs
Signed-off-by: Faraz Khoubsirat <58580514+farazkh80@users.noreply.github.com> # Conflicts: # cpp/tensorrt_llm/kernels/trtllmGenKernels/gemm/KernelRunner.cpp
|
/bot run --disable-fail-fast |
|
PR_Github #69761 [ run ] triggered by Bot. Commit: |
|
PR_Github #69761 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #69818 [ run ] triggered by Bot. Commit: |
|
PR_Github #69818 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #69976 [ run ] triggered by Bot. Commit: |
|
PR_Github #69976 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #70050 [ run ] triggered by Bot. Commit: |
|
PR_Github #70050 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #70154 [ run ] triggered by Bot. Commit: |
|
PR_Github #70154 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #70159 [ run ] triggered by Bot. Commit: |
|
PR_Github #70159 [ run ] completed with state |
…7 unit-test scope kernelLauncher.h dispatched W4A8/int8/int4 groupwise and per-channel kernels only for arch == 100 || arch == 103, and preprocess_weights_for_mixed_gemm() skipped weight interleaving only for sm_ == 100 || sm_ == 103 -- both leaving sm107 on the wrong path. Route both through the existing isSM100Family()/is_sm_100f() helpers, which already cover the sm100 family (100 <= sm < 110). Also bump triton_bmm's num_warps to 8 on sm107, matching its wider warp occupancy versus sm100/103. The cutlass_preprocessors and fpA_intB C++ hunks of the original fix are already covered by NVIDIA#17485. Signed-off-by: Faraz Khoubsirat <58580514+farazkh80@users.noreply.github.com>
…7 unit-test scope kernelLauncher.h dispatched W4A8/int8/int4 groupwise and per-channel kernels only for arch == 100 || arch == 103, and preprocess_weights_for_mixed_gemm() skipped weight interleaving only for sm_ == 100 || sm_ == 103 -- both leaving sm107 on the wrong path. Route both through the existing isSM100Family()/is_sm_100f() helpers, which already cover the sm100 family (100 <= sm < 110). Also bump triton_bmm's num_warps to 8 on sm107, matching its wider warp occupancy versus sm100/103. The cutlass_preprocessors and fpA_intB C++ hunks of the original fix are already covered by NVIDIA#17485. Signed-off-by: Faraz Khoubsirat <58580514+farazkh80@users.noreply.github.com> Co-authored-by: Yifei Zhang <219273404+yifeizhang-c@users.noreply.github.com>
…-up to #17485) (#18612) Signed-off-by: Faraz Khoubsirat <58580514+farazkh80@users.noreply.github.com> Co-authored-by: Yifei Zhang <219273404+yifeizhang-c@users.noreply.github.com> Co-authored-by: Bowen Fu <BowenFu@users.noreply.github.com>
Description
SM107 support for the CUTLASS GEMM dispatch paths and FP8 block-scale quantization kernels.
cutlass::arch::Sm100kernels, so this is mostly predicate widening:sm == 100 || sm == 103checks becomeisSM100Family()(a strict superset — SM100/SM103 select exactly the same paths as before, only SM107 is newly reachable). The build-system half landed in [None][infra] Recognize SM107 (Rubin) in build config and arch detection #17336.SF_VEC_SIZE/SF_OUTPUT_VEC_SIZE; defaults reproduce the previous 32-wide behavior exactly.fp8_quantize_1x128_packed_ue8m0gainsuse_r128c4_layout=True, switching its default output to the standard R128c4 scale layout. Its only in-tree consumer (deep_gemm) is updated in the same commit to request the legacy layout explicitly, so that path is unchanged.Test Coverage
test_fp8_quantize.pyvalidates the new R128c4 default (test_fp8_quantize_1x128_packed_ue8m0_r128c4*) and keeps the legacy deep_gemm contract undertest_..._legacy_layout. Dispatch changes are covered by the existing FP4 / FP8-rowwise / fpA_intB / FP8 block-scale GEMM suites. Requesting B200 and H100 stages — Hopper shares thefp8BlockScalingGemmdispatch andquantization.*templates, though no Hopper behavior should change.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.Dev Engineer Review
Sm100f.SF_VEC_SIZEandSF_OUTPUT_VEC_SIZE.(128, 32)support.fp8_quantize_1x128_packed_ue8m0layout as the default.QA Engineer Review
Modified or added test functions in
tests/unittest/_torch/thop/parallel/test_fp8_quantize.py:test_fp8_quantize_1x128_packed_ue8m0_matches_legacytotest_fp8_quantize_1x128_packed_ue8m0_r128c4.test_fp8_quantize_1x128_packed_ue8m0_padded_rows_are_zero.test_fp8_quantize_1x128_packed_ue8m0_r128c4_zero_blocks.test_fp8_quantize_1x128_packed_ue8m0_r128c4_padding_is_zero.test_fp8_quantize_1x128_packed_ue8m0_legacy_layout.Coverage in
tests/integration/test_lists/is not established from the available change summary.Verdict: needs follow-up.