Skip to content

[chore] Linking fixes to NVRTC wrapper #5189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AlessioNetti
Copy link
Contributor

This PR introduces two changes related to linking of the NVRTC wrapper:

  1. Bugfix: PR 4898 introduced a dependency on the NVPTX compiler library within the NVRTC wrapper, but no linking options were added to the respective CMakeLists.txt files. This PR adds the appropriate linking flags.
  2. Change: The NVRTC wrapper is linked against the static NVRTC libraries. These libraries are usually not available in general-purpose images (e.g., the PyTorch or TensorRT NGC images) but need to be installed explicitly via the CUDA toolkit. The dynamic NVRTC libraries, on the other hand, are usually provided out-of-the-box. As such, this PR changes linking of the NVRTC wrapper to use the dynamic NVRTC libraries.

Signed-off-by: Alessio Netti <netti.alessio@gmail.com>
Signed-off-by: Alessio Netti <netti.alessio@gmail.com>
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses linking issues with the NVRTC wrapper by (1) adding the missing NVPTX dependency to the NVRTC wrapper target and (2) switching from static to dynamic linking for NVRTC libraries to better align with available toolkit distributions.

  • Added NVPTX_LIB to the NVRTC wrapper's target_link_libraries
  • Updated global CMake variables to use dynamic NVRTC libraries and introduced NVPTX_LIB for proper linkage

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
cpp/tensorrt_llm/kernels/decoderMaskedMultiheadAttention/decoderXQAImplJIT/nvrtcWrapper/CMakeLists.txt Added NVPTX_LIB in the target link libraries to fix missing dependency linking
cpp/CMakeLists.txt Changed NVRTC linking from static to dynamic libraries and set up NVPTX_LIB appropriately
Comments suppressed due to low confidence (2)

cpp/tensorrt_llm/kernels/decoderMaskedMultiheadAttention/decoderXQAImplJIT/nvrtcWrapper/CMakeLists.txt:20

  • Ensure that the addition of ${NVPTX_LIB} integrates well with the rest of the CUDA libraries and that the link ordering honors dependency requirements across different build configurations.
nvrtc_wrapper_src PUBLIC ${NVPTX_LIB} ${NVRTC_LIB} ${NVRTC_BUILTINS_LIB} ${CUDA_DRV_LIB}

cpp/CMakeLists.txt:142

  • Review that switching NVRTC and NVRTC_BUILTINS_LIB to dynamic linking (lines 143-144) is consistent with expected usage across all configurations, and that dependent components are compatible with these changes.
set(NVPTX_LIB CUDA::nvptxcompiler_static)

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 13, 2025

@tongyuantongyu could you please take a look at these changes?

Copy link
Member

@tongyuantongyu tongyuantongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 13, 2025

/bot run

@nv-guomingz nv-guomingz added the Community want to contribute PRs initiated from Community label Jun 13, 2025
@tensorrt-cicd
Copy link
Collaborator

PR_Github #8787 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

PR_Github #8787 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #6383 completed with status: 'FAILURE'

Signed-off-by: Robin Kobus <19427718+Funatiq@users.noreply.github.com>
@Funatiq
Copy link
Collaborator

Funatiq commented Jun 13, 2025

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #8803 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

PR_Github #8803 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #6391 completed with status: 'FAILURE'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community want to contribute PRs initiated from Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants