-
Notifications
You must be signed in to change notification settings - Fork 61
Few CMake improvements #2204
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
Few CMake improvements #2204
Conversation
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 removes the BUILD_SPLIT_KERNEL_LIB option and its associated logic for splitting kernels into multiple libraries, which was only needed for older compiler versions. It also updates the project version to match PyTorch 2.10.0, fixes a comment typo, and improves the header installation macro.
Key changes:
- Removes split kernel library build paths for both Windows and Linux
- Updates project version from 2.3.0 to 2.10.0
- Simplifies the
install_xpu_headersmacro withCONFIGURE_DEPENDS
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/BuildOnWindows.cmake | Removes 200+ lines of split kernel library logic for Windows builds |
| src/BuildOnLinux.cmake | Removes 87 lines of split kernel library logic for Linux builds |
| src/ATen/CMakeLists.txt | Simplifies header installation macro and moves mkl file glob inside conditional |
| cmake/Modules/FindSYCL.cmake | Corrects comment from "FindCUDA.cmake" to "FindSYCL.cmake" |
| cmake/Modules/FindONEMKL.cmake | Refactors MKL library list construction and improves warning message |
| CMakeLists.txt | Updates project version to 2.10.0 and removes BUILD_SPLIT_KERNEL_LIB variable |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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, please confirm that we could remove BUILD_SPLIT_KERNEL_LIB
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.
Overall LGTM. The only thing we need to discuss is the removal of BUILD_SPLIT_KERNEL_LIB. As I understand it, this flag was originally used to
- Work around the binary size when too many AOT targets were enabled, however, this is no longer necessary since the compiler now supports device code suppression feature.
- Facilitate debugging - some developers might still use this flag to debug. It is useful for reducing incremental build time, especially when debugging ATen XPU ops code.
Personally, I haven't used BUILD_SPLIT_KERNEL_LIB recently, so I can't decide whether it should be removed. Let's ask for opinions from others.
I suggest you split this PR into two separate ones. I can help land the other changes, and we can keep the BUILD_SPLIT_KERNEL_LIB removal PR for further discussion.
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. Thanks~
|
Thanks for review. I agree that it will be better to split this PR and continue discussion about if(CMAKE_BUILD_TYPE MATCHES "(Debug|RelWithDebInfo)")
set(BUILD_SEPARATE_OPS TRUE)
endif()For debug builds if(BUILD_SEPARATE_OPS)
...
elseif(BUILD_SPLIT_KERNEL_LIB OR __INTEL_LLVM_COMPILER LESS 20250004 OR ICX_DATE LESS 20241205)
...
else()
...
endif()Continued in #2208 |
|
It fails on CI, but as these changes affect only build files it's unlikely that this PR introduced functional regressions. @guangyey can this be somehow marked as not relevant or should I re-run CI until all tests are passing? |
|
Land this due to the low risk. |
`BUILD_SPLIT_KERNEL_LIB` looks like workaround, which is no longer needed in new compiler versions. From release notes > The compiler team introduced device image compression in version 2025.0.1, and this feature is now enabled in oneMKL 2025.2 builds. It facilitates the reduction in size of SYCL libraries with large kernels by approximately 12% to 40% for both static and dynamic libraries on Linux, as well as dynamic libraries on Windows. Also this flag can't be used for Debug/RelWithDefInfo builds, as for these configs `BUILD_SEPARATE_OPS` takes precedence ```CMake if(CMAKE_BUILD_TYPE MATCHES "(Debug|RelWithDebInfo)") set(BUILD_SEPARATE_OPS TRUE) endif() ... if(BUILD_SEPARATE_OPS) ... elseif(BUILD_SPLIT_KERNEL_LIB OR __INTEL_LLVM_COMPILER LESS 20250004 OR ICX_DATE LESS 20241205) ... else() ... endif() ``` Continuation of #2204
CONFIGURE_DEPENDSto track these headers