Support static linking of libcudnn#182
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCMake cuDNN module refactored to support static and shared builds: version detection reads ChangescuDNN CMake Configuration for Static and Shared Builds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@hwanseoc |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmake/cuDNN.cmake (1)
87-91: 💤 Low valueUse "CUDNN" as the package name for clearer error messages.
The first argument should be the package name, not "LIBRARY". Currently, if discovery fails, users will see "Could NOT find LIBRARY" instead of "Could NOT find CUDNN", making it harder to diagnose configuration issues.
include (FindPackageHandleStandardArgs) find_package_handle_standard_args( - LIBRARY REQUIRED_VARS + CUDNN REQUIRED_VARS CUDNN_INCLUDE_DIR ${CUDNN_LIBRARY_VAR} )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/cuDNN.cmake` around lines 87 - 91, The find_package_handle_standard_args invocation uses "LIBRARY" as the package name which yields misleading messages; change the first argument to "CUDNN" so failure messages read "Could NOT find CUDNN". Edit the call to find_package_handle_standard_args (the function named find_package_handle_standard_args) and pass CUDNN as the package name while keeping the same REQUIRED_VARS list (CUDNN_INCLUDE_DIR and ${CUDNN_LIBRARY_VAR}) so error reporting correctly references CUDNN.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/cuDNN.cmake`:
- Around line 161-171: The generator expression is using the literal token
CUDNN_STATIC instead of the variable value, causing the static-only clauses to
always evaluate true; update each $<BOOL:CUDNN_STATIC> occurrence to use
variable expansion (e.g., $<BOOL:${CUDNN_STATIC}>), and likewise ensure the
static-libraries clause uses $<BOOL:${CUDNN_STATIC}> so the
--whole-archive/--no-whole-archive and CUDA::cublasLt_static /
CUDA::nvrtc_static / ZLIB::ZLIB selection are only applied when the CUDNN_STATIC
variable is truthy.
- Line 121: The current find_package(ZLIB) call can leave ZLIB unresolved but
later references ZLIB::ZLIB for static builds; update the CMake logic so ZLIB is
required when performing static builds: detect the static-build condition used
in this CMake script (e.g., BUILD_SHARED_LIBS OFF or your BUILD_STATIC option)
and call find_package(ZLIB REQUIRED) in that branch (otherwise keep the
non-REQUIRED call for optional builds), so that missing ZLIB fails at configure
time instead of producing a confusing "target ZLIB::ZLIB not found" at link
time.
---
Nitpick comments:
In `@cmake/cuDNN.cmake`:
- Around line 87-91: The find_package_handle_standard_args invocation uses
"LIBRARY" as the package name which yields misleading messages; change the first
argument to "CUDNN" so failure messages read "Could NOT find CUDNN". Edit the
call to find_package_handle_standard_args (the function named
find_package_handle_standard_args) and pass CUDNN as the package name while
keeping the same REQUIRED_VARS list (CUDNN_INCLUDE_DIR and ${CUDNN_LIBRARY_VAR})
so error reporting correctly references CUDNN.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 241e37ae-8491-4d78-843d-2058cc2a2a49
📒 Files selected for processing (1)
cmake/cuDNN.cmake
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@cudnn-ci-bot run |
|
🚀 Running mirror pipeline Branch: cudnn-gh/pr-182-a143d19 |
3 participants
Since most FindCUDNN.cmake's static feature seems to be broken from cudnn v8 I've like to fix it.
libcudnn_static.a isn't included at least from v9 and there is more tricks needed to link static cudnn documented in:
https://docs.nvidia.com/deeplearning/cudnn/installation/latest/build-run-cudnn.html#running-a-cudnn-dependent-program
In this PR, I'll add 2 hint flags:
CUDNN_STATICto enabled static linking featureCUDNN_SKIP_PRECOMPILED_LINKto disable linking tocudnn_engines_precompiledwhich sometimes cause overflowing of 2GiB relocation limit of libcudnn's 32bit binary which is reported in https://discuss.pytorch.org/t/libtorch-cxx11-abi-static-library/211309I've tried
working and result of
lddseems to be minimum as I expect:Summary by CodeRabbit