[https://nvbugs/6390307][fix] Update trtllm-gen FMHA JIT libraries - #15974
Conversation
1dd1179 to
83709cc
Compare
|
/bot run --post-merge --disable-fail-fast |
📝 WalkthroughWalkthroughThis PR modifies a diagnostic check in KernelTraits.h to suppress an informational hint for generation kernels that skip softmax, updates the corresponding prebuilt trtllmGen library binaries (aarch64 and x86_64) via Git LFS pointers, and removes now-obsolete test waivers for the related skip-softmax attention test. ChangesSkip-softmax attention fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 (1)
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/trtllmGen_fmha_export/KernelTraits.h (1)
182-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the local as
const.
isGenerationSkipsSoftmaxis never reassigned after initialization.♻️ Proposed fix
- bool isGenerationSkipsSoftmax = + bool const isGenerationSkipsSoftmax = options.mSkipsSoftmaxWhenPossible && !isContextKernel(options.mFmhaKernelType);As per coding guidelines, "A variable that is not modified after its initialization should be declared as
const" and should use east-const style (int const x).🤖 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/trtllmGenKernels/fmha/trtllmGen_fmha_export/KernelTraits.h` around lines 182 - 184, The local flag in KernelTraits::isGenerationSkipsSoftmax is only initialized and never reassigned, so declare it as const using east-const style. Update the declaration in the generation-kernel softmax check to use a const local while keeping the existing condition logic unchanged.Source: Coding guidelines
🤖 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/trtllmGenKernels/fmha/trtllmGen_fmha_export/KernelTraits.h`:
- Around line 182-184: The local flag in KernelTraits::isGenerationSkipsSoftmax
is only initialized and never reassigned, so declare it as const using
east-const style. Update the declaration in the generation-kernel softmax check
to use a const local while keeping the existing condition logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 38e1df50-b447-424f-af3e-089cc7c7c45e
📒 Files selected for processing (6)
cpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/lib/aarch64-linux-gnu/libTrtLlmGen.acpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/lib/aarch64-linux-gnu/libTrtLlmGenFmhaLib.acpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/lib/x86_64-linux-gnu/libTrtLlmGen.acpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/lib/x86_64-linux-gnu/libTrtLlmGenFmhaLib.acpp/tensorrt_llm/kernels/trtllmGenKernels/fmha/trtllmGen_fmha_export/KernelTraits.htests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
PR_Github #57976 [ run ] triggered by Bot. Commit: |
|
PR_Github #57976 [ run ] completed with state
|
7cec8b7 to
944a450
Compare
|
/bot run --post-merge --disable-fail-fast |
|
PR_Github #58250 [ run ] triggered by Bot. Commit: |
|
/bot kill |
|
PR_Github #58251 [ kill ] triggered by Bot. Commit: |
|
PR_Github #58250 [ run ] completed with state |
|
PR_Github #58251 [ kill ] completed with state |
Refresh x86_64 and aarch64 FMHA JIT libraries for skip-softmax generation and sm_103a NVRTC linking. Synchronize FmhaAutoTuner, FmhaOptions, and KernelTraits with the matching FMHA snapshot. Retain all other exported headers and cubins. Remove the related test waivers. Refs NVBug 6388157 Refs NVBug 6390307 Refs TRTLLM-12988 Refs TRTLLM-12990 Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
944a450 to
501d7cd
Compare
|
/bot run --post-merge --disable-fail-fast |
|
PR_Github #58257 [ run ] triggered by Bot. Commit: |
|
PR_Github #58257 [ run ] completed with state
|
|
/bot run --reuse-test |
|
PR_Github #58388 [ run ] triggered by Bot. Commit: |
|
PR_Github #58388 [ run ] completed with state
|
|
/bot run --reuse-test |
|
PR_Github #58455 [ run ] triggered by Bot. Commit: |
|
PR_Github #58455 [ run ] completed with state |
|
✅ LFS objects already in storage (4 files) — no sync needed. These LFS-tracked files are already present in this repository's LFS storage:
|
Description
Refresh the x86_64 and aarch64 FMHA JIT static libraries from the matching FMHA source snapshot.
numInstsKv=2for skip-softmax generation kernels.sm_103ain multi-instance NVRTC linking.FmhaAutoTuner.h,FmhaOptions.h, andKernelTraits.hwith the libraries.Addresses https://nvbugs/6390307 and https://nvbugs/6428176
Test Coverage
tekit:latestReleasebuild_wheel_targetspassed.PR Checklist