Skip to content

Conversation

AlessioNetti
Copy link
Contributor

@AlessioNetti AlessioNetti commented Jun 13, 2025

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 introduces a --nvrtc_dynamic_linking flag for the build_wheel.py script, which (optionally) allows using the dynamic NVRTC libraries as opposed to the static ones.

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 <[email protected]>
@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'

@lowsfer
Copy link
Member

lowsfer commented Jun 16, 2025

By design, we meant to statically link all three libs, but somehow we missed two of them.

Our intent was to link nvrtc/ptx compilers statically and avoid runtime dependency on these *.so, which can causes complex compatibility problems among nvrtx, ptx compiler, and driver.

Our build script will automatically install the required static libs for building.

@AlessioNetti AlessioNetti changed the title [chore] Linking fixes to NVRTC wrapper [chore] Allow configuring linking of NVRTC wrapper Jun 17, 2025
@AlessioNetti
Copy link
Contributor Author

I just implemented a --nvrtc_dynamic_linking option for the build_wheel.py script, which allows to optionally select the dynamic NRTC libraries for linking of the NVRTC wrapper. This is disabled by default, meaning that the static libraries are used unless explicitly requested otherwise.

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 17, 2025

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #9184 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

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

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 17, 2025

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #9211 [ run ] triggered by Bot

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 24, 2025

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #9737 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

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

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 25, 2025

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #9820 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

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

@Funatiq
Copy link
Collaborator

Funatiq commented Jun 25, 2025

/bot run

@tensorrt-cicd
Copy link
Collaborator

PR_Github #9921 [ run ] triggered by Bot

@tensorrt-cicd
Copy link
Collaborator

PR_Github #9921 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #7324 completed with status: 'SUCCESS'

@Funatiq Funatiq merged commit 7e681fb into NVIDIA:main Jun 26, 2025
3 checks passed
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 9, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 10, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 10, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 10, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 10, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 11, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 11, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
dominicshanshan pushed a commit to dominicshanshan/TensorRT-LLM that referenced this pull request Jul 11, 2025
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
Co-authored-by: Robin Kobus <[email protected]>
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.

6 participants