Repository navigation
[#20507][fix] Number Sm107a as 5 to match the prebuilt trtllm-gen archives - #20518
lucifer1004 wants to merge 3 commits into
Conversation
…en archives The prebuilt trtllm-gen FMHA archives use 4 for Sm105a and 5 for Sm107a. With TLLM_RUBIN_FEATURES the exported CudaArchDecl.h numbered Sm107a as 4, so the archives treated it as Sm105a. Add a CPU-only gtest that checks the numbering against libTrtLlmGen.a. Signed-off-by: Zihua Wu <13583761+lucifer1004@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Configure and check archive ABI cpp/tests/unit_tests/kernels/CMakeLists.txt, cpp/tests/unit_tests/kernels/trtllmGenCudaArchAbiTest.cpp |
CMake adds a non-Windows CPU test target with direct archive linkage. The test expects 512 for Sm100a, Sm100f, and Sm103a, and 576 for Sm107a. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: qijune
Merge Risk: 
Minimal · up to 5843f
-
No actionable merge-blocking risk was identified in the reviewed change.
Pre-merge checks: 
5
-
Full details: Description check
-
Explanation
The description includes complete Description, Test Coverage, and PR Checklist sections. It clearly explains the issue, solution, test scope, local validation, and remaining limitation that a full CMake build was not run.
Full details: Linked Issues check
-
Explanation
Issue
#20507requires matchingCudaArchvalues across the FMHA interface and the prebuilt archives. The PR reportsSm107a = 5underTLLM_RUBIN_FEATURES. The CPU-only test links the FMHA interface directly tolibTrtLlmGen.aand verifies thatSm107areturns the archive's 576-TMEM-column result. It also verifies the unchangedSm100a,Sm100f, andSm103amappings. The exported header is excluded from direct inspection, but the reported passing test and its failure with the old header provide independent behavior evidence.
Full details: Out of Scope Changes check
-
Explanation
The changed CMake file registers the ABI test, and the test directly validates the
#20507archive boundary. These changes support the linked issue. No unrelated implementation, dependency, or configuration change is shown.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
@yunruis, could you help to review this PR as trt-llm-torch-attention-devs? Thanks~ Assignment explanation
Affinity and workload are snapshots at selection, not live counters. Existing reservations on this PR are counted once. |
|
@yunruis thanks for the review. Agreed: |
|
/bot run |
|
PR_Github #78962 [ run ] triggered by Bot. Commit: |
|
PR_Github #78962 [ run ] completed with state
|
|
CI #78962 failed outside this PR. The only hard failure is on Re-running. |
|
/bot run |
|
PR_Github #79028 [ run ] triggered by Bot. Commit: |
|
PR_Github #79028 [ run ] completed with state
|
|
CI #79028 did not get to run tests: the |
|
/bot run |
|
PR_Github #79041 [ run ] triggered by Bot. Commit: |
|
PR_Github #79041 [ run ] completed with state
|
Signed-off-by: Zihua Wu <13583761+lucifer1004@users.noreply.github.com>
|
The last CI run didn't get to the multi-GPU stages. No test failures were counted in it. A CPU arm unit test, I merged current main (557ddca) so the multi-GPU stages pick up the recent waives, including the GPT-OSS eagle3 accuracy case (https://nvbugs/6962175). The PR diff is unchanged. Re-running. |
|
/bot run |
|
PR_Github #79358 [ run ] triggered by Bot. Commit: |
|
PR_Github #79358 [ run ] completed with state
|
|
The last run failed for infrastructure reasons and no test failure was reported: the DGX_B200 stages could not reach the Slurm login node, and fail-fast then interrupted the remaining stages. The B200 stages are running normally again on other PRs, so re-running. |
|
/bot run |
|
PR_Github #79678 [ run ] triggered by Bot. Commit: |
|
PR_Github #79678 [ run ] completed with state
|
|
The last run reported no test failures, but it stopped before the multi-GPU stages. On A10, |
|
/bot run |
|
PR_Github #80118 [ run ] triggered by Bot. Commit: |
|
PR_Github #80118 [ run ] completed with state
|
|
Correction to my previous comment: the multi-GPU stages were not blocked by the A10 timeout. They are gated on the |
Signed-off-by: Zihua Wu <13583761+lucifer1004@users.noreply.github.com>
|
/bot run |
|
PR_Github #81434 [ run ] triggered by Bot. Commit: |
|
PR_Github #81434 [ run ] completed with state
|
Description
Fixes #20507.
The trtllm-gen FMHA target is compiled with
TLLM_RUBIN_FEATURES, and the exportedtrtllm/gen/CudaArchDecl.hnumberedCudaArch::Sm107aas 4. The prebuiltfmha/lib/<arch>/libTrtLlmGen.aandlibTrtLlmGenFmhaLib.awere compiled with an enum that also hasSm105a: there 4 isSm105aand 5 isSm107a. So on SM107 everyCudaArchvalue thatTllmGenFmhaKernelpasses into the archives (throughoptions.mCudaArch, for example inwarmupOneKerneland theFmhaAutoTunerpaths) is read asSm105a. Archive functions that switch on the architecture then fail; for examplegetMaxNumTmemColsthrowsUnsupported CUDA arch: 105a.This PR sets
Sm107a = 5so both sides use the same numbering. Other values are unchanged (0..3 already match), andSm107ais only defined whenTLLM_RUBIN_FEATURESis set, so non-Rubin builds are unaffected.Test Coverage
cpp/tests/unit_tests/kernels/trtllmGenCudaArchAbiTest.cpp. It links only the prebuiltlibTrtLlmGen.a(nolibtensorrt_llm, no GPU) and checks that the archive'sgetMaxNumTmemColsreturns 512 forSm100a,Sm100fandSm103aand 576 forSm107a. It is incpp/tests/unit_tests/kernels, so the existingcpp/test_unit_tests.py::test_unit_tests[kernels-*]stages pick it up.trtllm_gen_fmha_interface. With this change it passes. With the old header it fails withC++ exception with description "Unsupported CUDA arch: 105a".PR Checklist
Dev Engineer Review
Sm107ais explicitly assigned value5underTLLM_RUBIN_FEATURES. This matches the prebuilt FMHA archive numbering and preserves the preceding enum values.libTrtLlmGen.aarchive. CMake excludes the test on Windows.QA Engineer Review
trtllmGenCudaArchAbiTest.cppand registered the test incpp/tests/unit_tests/kernels/CMakeLists.txt.MaxNumTmemColsMatchesArchivechecks thatSm100a,Sm100f, andSm103areturn 512, andSm107areturns 576 from the archive.Per-File QA Perspective
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/trtllmGen_fmha_export/trtllm/gen/CudaArchDecl.h: Changes RubinSm107ato value5. Verify that SM107 archive calls use the intended architecture and that non-Rubin enum values remain unchanged.cpp/tests/unit_tests/kernels/CMakeLists.txt: Adds a non-Windows test target with direct FMHA archive linkage. Verify archive discovery and linkage on supported platforms.cpp/tests/unit_tests/kernels/trtllmGenCudaArchAbiTest.cpp: Checks archive TMEM limits for four architecture values. The test is not listed in the integration CI or manual-QA lists.