Repository navigation
[None][fix] Add 107 to ARCHITECTURES_COMPATIBILITY_BASE so -a 107-real emits SM107 code - #19069
100milliongold wants to merge 1 commit into
Conversation
Architectures in neither ARCHITECTURES_COMPATIBILITY_BASE nor ARCHITECTURES_NO_COMPATIBILITY are folded to the largest base sharing their major version. 107 was in neither, so -a 107-real silently became 100 and no SM107 code was ever emitted. 9f92665 added 107 to the default architecture list and to ARCHITECTURES_WITH_KERNELS but not to this one. With this line, 133 targets pick up compute_107f and the linked library gains 72 sm_107 ELFs; every other architecture is unchanged. Refs NVIDIA#19067 Signed-off-by: 김재억 <gadian88@gmail.com>
|
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)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe CUDA configuration now includes architecture ChangesCUDA architecture compatibility
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to SM107 normalization now produces SM107 code as intended, while existing architectures remain unchanged. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| # Compatibility low bounds: Always compile kernels for these architectures. 86 | ||
| # is enabled to avoid perf regression when using 80 kernels. | ||
| set(ARCHITECTURES_COMPATIBILITY_BASE 80 86 90 100 120) | ||
| set(ARCHITECTURES_COMPATIBILITY_BASE 80 86 90 100 107 120) |
There was a problem hiding this comment.
Adding 107 here means all kernels will be build for this arch, when enabled. That leads to longer build time and binary size. And not all kernels can benefit from targeting 107a. That's why 107 is not put here. Please refer to this comment in the original PR for more details.
To target 107a only for specific kernel, please refer to this comment.
|
Thanks for the detailed explanation, @zhangcl, and for linking the original discussion in #17336. You are right, and I am withdrawing this PR. My premise was wrong, and I want to record exactly how I got it wrong so the next person does not repeat it. What I measured, and why it was misleading I concluded "no SM107 code is generated" from Both cubins are the same size, have identical ELF headers, identical The build log answers it directly, and it was there the whole time:
list(APPEND CMAKE_CUDA_ARCHITECTURES_NORMALIZED "${CUDA_ARCH}f-real")So Conclusion Folding 107 into the 100 compatibility base is deliberate, and adding 107 to I am opening a separate, narrower PR for one place where the family suffix is dropped for 10x architectures other than |
|
Follow-up as mentioned above: #19095 — DeepEP drops the family/arch suffix for 10x architectures other than |
Description
-a 107-realis accepted and reachesCMakeCache.txtintact, but no SM107 code is everproduced.
setup_cuda_architectures()normalizes requested architectures againstAn architecture in neither list is folded to the largest base sharing its major version.
107 is in neither, and
107 / 10 == 100 / 10, so it silently becomes 100. No warning isemitted.
9f92665 ("[None][infra] Recognize SM107 (Rubin) in build config and arch detection")
added 107 to the default architecture list and to
ARCHITECTURES_WITH_KERNELS, but notto this one, so the normalization step still removes it.
Background and the fuller trace are in #19067.
Test Coverage
Configured with
-DCMAKE_CUDA_ARCHITECTURES="90-real;100-real;103-real;107-real",counting gencode flags across every generated
flags.make:compute_90acompute_100fcompute_103acompute_107fNothing else changes, so architectures that already worked are unaffected.
A full
--cpp_onlybuild then completes and the resulting shared library really doescarry SM107 code. Same tree, same flags, only this one line differing:
libtensorrt_llm.sosize(
cuobjdump --list-elf, counted on the linked library.) Every previously presentarchitecture is unchanged; the 40 MB difference is the SM107 code.
For a single target compiled by hand with the flags CMake generated for it
(
tensorrt_llm/common/cudaFp8Utils.cu):Environment: nvcc 13.4 V13.4.59, gcc 13.3.0, Ubuntu 24.04, no GPU used (compile only).
A question rather than an assumption
I cannot tell whether the fold is currently deliberate.
generate_kernels.pycontains nooccurrence of
107and routes it into the SM100 family throughrange(100, 110), andthe Rubin kernels that do ship are precompiled
sm107acubins rather than sources. Ifthe intent is "build SM100 code and run it on Rubin until Rubin sources land", then this
PR is premature and the real issue is only that
-a 107-realsilently does nothing.Please close this if that is the case. I opened it because the one-liner is small enough
to judge quickly, not because I am confident it is wanted.
PR Checklist
Dev Engineer Review
Adds architecture
107toARCHITECTURES_COMPATIBILITY_BASE. This preserves-a 107-realand enables SM107 code generation instead of normalizing to SM100. The change is limited to one CMake list and does not affect existing architectures.QA Engineer Review
No test changes.
Per-File QA Perspective
cpp/cmake/modules/cuda_configuration.cmake: Verify SM107 output for-a 107-realand unchanged output for existing architectures. Reported validation showed 133compute_107fflags, 72 linked-library SM107 ELF entries, and a successful CUDA 13.4 compile-only build.