-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Alessio Netti <netti.alessio@gmail.com>
Signed-off-by: Alessio Netti <netti.alessio@gmail.com>
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 <19427718+Funatiq@users.noreply.github.com>
/bot run |
PR_Github #8803 [ run ] triggered by Bot |
PR_Github #8803 [ run ] completed with state |
This PR introduces two changes related to linking of the NVRTC wrapper:
CMakeLists.txt
files. This PR adds the appropriate linking flags.