-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[chore] Allow configuring linking of 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
Conversation
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Alessio Netti <[email protected]>
There was a problem hiding this 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)
@tongyuantongyu could you please take a look at these changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
/bot run |
PR_Github #8787 [ run ] triggered by Bot |
PR_Github #8787 [ run ] completed with state |
...rt_llm/kernels/decoderMaskedMultiheadAttention/decoderXQAImplJIT/nvrtcWrapper/CMakeLists.txt
Outdated
Show resolved
Hide resolved
Signed-off-by: Robin Kobus <[email protected]>
/bot run |
PR_Github #8803 [ run ] triggered by Bot |
PR_Github #8803 [ run ] completed with state |
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. |
…tatic NVRTC linking Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Alessio Netti <[email protected]>
Signed-off-by: Alessio Netti <[email protected]>
I just implemented a |
Signed-off-by: Robin Kobus <[email protected]>
/bot run |
PR_Github #9184 [ run ] triggered by Bot |
PR_Github #9184 [ run ] completed with state |
/bot run |
PR_Github #9211 [ run ] triggered by Bot |
/bot run |
PR_Github #9737 [ run ] triggered by Bot |
PR_Github #9737 [ run ] completed with state |
/bot run |
PR_Github #9820 [ run ] triggered by Bot |
PR_Github #9820 [ run ] completed with state |
/bot run |
PR_Github #9921 [ run ] triggered by Bot |
PR_Github #9921 [ run ] completed with state |
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
Signed-off-by: Alessio Netti <[email protected]> Signed-off-by: Robin Kobus <[email protected]> Co-authored-by: Robin Kobus <[email protected]>
This PR introduces two changes related to linking of the NVRTC wrapper:
CMakeLists.txt
files. This PR adds the appropriate linking flags.--nvrtc_dynamic_linking
flag for thebuild_wheel.py
script, which (optionally) allows using the dynamic NVRTC libraries as opposed to the static ones.