[https://nvbugs/5542862][fix] Upgrade fmha_v2. #8364
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #21353 [ run ] triggered by Bot |
|
PR_Github #21353 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #21424 [ run ] triggered by Bot |
|
PR_Github #21424 [ run ] completed with state |
📝 WalkthroughWalkthroughThis PR updates Git LFS pointers for approximately 80 compiled CUDA binaries within contextFusedMultiHeadAttention kernels, reflecting rebuilt or recompiled artifacts with updated checksums and file sizes. Additionally, a debug print statement is added to log function lookups during kernel initialization. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes The changes are predominantly homogeneous Git LFS pointer metadata updates across compiled binary artifacts. The pattern is consistent across all files (oid and size field replacements with no logic modifications). The single source code change is a straightforward debug print statement. Visual scanning and spot-checking a few representative file updates suffices for validation. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/contextFusedMultiHeadAttention/fused_multihead_attention_v2.cpp (1)
154-156: Out-of-bounds risk: incorrect device index bound check.device_id == 32 will index s_factory[32] out of range. Also guard against negative ids.
Apply:
- TLLM_CHECK(device_id <= 32); + TLLM_CHECK(device_id >= 0 && device_id < 32);Optionally also check the cudaGetDevice return code and fail early.
- cudaGetDevice(&device_id); + TLLM_CUDA_CHECK(cudaGetDevice(&device_id));
♻️ Duplicate comments (13)
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_64_256_S_q_paged_kv_128_softcapping_tma_ws_sm90.cubin.cpp (1)
2-3: Metadata-only LFS pointer bump.No functional code here. Same operational note as above about LFS smudge before build/analysis.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_128_128_S_q_paged_kv_40_sm89.cubin.cpp (1)
2-3: Metadata-only LFS pointer bump.LGTM. Covered by the verification script in the first comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_fp16_fp32_64_128_S_q_paged_kv_128_alibi_tma_ws_sm90.cubin.cpp (1)
2-3: Metadata-only LFS pointer bump.No code/API changes; ensure LFS objects are available in CI/dev as noted.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_fp16_64_128_S_qkv_128_tma_ws_sm90.cubin.cpp (1)
2-3: Metadata-only LFS pointer bump.Looks good; same LFS smudge prerequisite applies.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_64_32_S_q_paged_kv_72_sm89.cubin.cpp (1)
2-3: Same as prior file: pointer refresh OK; fix CI to LFS-checkout before clang/build.Static analysis errors stem from reading LFS pointer text. Ensure git lfs checkout happens pre-analysis/build; see script in the first comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_64_256_S_qkv_128_sage_64_64_256_output_bf16_tma_ws_sm90.cubin.cpp (1)
2-3: Pointer metadata bump acknowledged; CI/static-analysis note as above.No functional code here. Please rely on the LFS workflow and pre-clang checkout; refer to the verification script posted earlier.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_bf16_64_32_S_qkv_128_sm90.cubin.cpp (1)
2-3: LGTM — LFS pointer updated.Same CI action item: run git lfs fetch/checkout before static analysis and builds to avoid false clang errors.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_64_32_S_q_paged_kv_192_sm89.cubin.cpp (1)
2-3: OK to merge — pointer-only change.Repeat of the LFS checkout requirement for CI to silence bogus clang diagnostics; see the first file’s script.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_64_32_S_qkv_80_sage_64_32_32_output_fp16_sm89.cubin.cpp (1)
2-3: Same as prior: metadata-only LFS pointer update; LGTM.Covered by the CI/LFS preflight note in the first cubin file comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_64_32_S_qkv_160_sm89.cubin.cpp (1)
2-3: Same as prior: metadata-only LFS pointer update; LGTM.Covered by the CI/LFS preflight note in the first cubin file comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_128_128_S_q_paged_kv_48_sm89.cubin.cpp (1)
2-3: Same as prior: metadata-only LFS pointer update; LGTM.Covered by the CI/LFS preflight note in the first cubin file comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_e4m3_fp32_64_32_S_q_kv_128_sm89.cubin.cpp (1)
2-3: Same as prior: metadata-only LFS pointer update; LGTM.Covered by the CI/LFS preflight note in the first cubin file comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_bf16_64_128_S_q_kv_128_softmax_tma_ws_sm90.cubin.cpp (1)
2-3: Same as prior: metadata-only LFS pointer update; LGTM.Covered by the CI/LFS preflight note in the first cubin file comment.
🧹 Nitpick comments (4)
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_fp16_fp32_64_128_S_qkv_128_sm90.cubin.cpp (1)
2-3: Metadata-only LFS pointer bump.All good. If desired, attach a brief build manifest (CUDA/NVCC, ptxas, CUTLASS/FlashAttention rev) in the PR description for traceability.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_fp16_fp32_64_128_S_q_paged_kv_128_tma_ws_sm90.cubin.cpp (1)
2-3: LGTM: LFS pointer metadata updated only.Static-analysis errors are expected on pointer stubs; build uses the real LFS content. Please also confirm locally that:
- CI pulls LFS blobs for these paths, and
- the actual .cpp content carries the 2025 NVIDIA Apache-2.0 header per guidelines; consider adding a brief build-provenance note (toolchain, flags, source commit) in the PR description for traceability of these OIDs.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/cubin/fmha_v2_flash_attention_bf16_64_128_S_q_kv_128_tma_ws_sm90.cubin.cpp (1)
2-3: LFS metadata update acknowledged; no code change.Same note: clang diagnostics stem from pointer stubs; ensure CI performs git‑lfs checkout before compiling/analyzing. Optionally confirm sm90/TMA cubin matches runtime selection. See the verification script in the first comment.
cpp/tensorrt_llm/kernels/contextFusedMultiHeadAttention/fused_multihead_attention_v2.cpp (1)
89-93: Replace printf with project logger and gate the noise.Direct stdout in core loading path is noisy and bypasses configured log levels. Use the logger (e.g., TLLM_LOG_DEBUG/INFO) and/or guard behind a verbose flag.
Apply:
- printf("Getting function %s\n", kernelMeta.mFuncName); + // Verbose diagnostic: kernel function resolved from cubin + TLLM_LOG_DEBUG((std::string("Getting function ") + kernelMeta.mFuncName).c_str());As per coding guidelines.
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #21702 [ run ] triggered by Bot. Commit: |
|
PR_Github #21702 [ run ] completed with state |
… (#8499) Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com> Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
Signed-off-by: Yuxian Qiu <142763828+yuxianq@users.noreply.github.com>
3 participants
Summary by CodeRabbit
Description
Test Coverage
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)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
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
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
Details
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.