Enabled LTO (Link Time Optimization)#9254
Conversation
Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
…UI library. - Unlike CMake, Bazel builds Qt from the source code, so Bazel can use LTO for Qt. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables Link-Time Optimization (LTO) for both Bazel and CMake builds to improve performance. For CMake, it introduces logic to detect operating systems where LTO is known to cause issues (Debian 11, Rocky Linux 8) and disables it accordingly. It also correctly disables LTO for the gui target, which was causing crashes. The changes are well-implemented. I've provided a couple of suggestions for the main CMakeLists.txt to improve the clarity of the build logs and the maintainability of the OS-checking logic.
| # Check for Debian 11 (Bullseye) | ||
| if(OS_ID STREQUAL "debian" AND OS_VERSION_ID STREQUAL "11") | ||
| set(LTO_UNSUPPORTED_OS TRUE) | ||
| message(STATUS "Detected Debian 11: LTO will be disabled due to known build issues") | ||
| endif() | ||
|
|
||
| # Check for Rocky Linux 8 | ||
| if(OS_ID STREQUAL "rocky" AND OS_VERSION_ID MATCHES "^8") | ||
| set(LTO_UNSUPPORTED_OS TRUE) | ||
| message(STATUS "Detected Rocky Linux 8: LTO will be disabled due to known build issues") | ||
| endif() |
There was a problem hiding this comment.
The separate checks for different unsupported operating systems can be combined into a single if statement using OR. This would make the code more concise and easier to extend with more unsupported OS versions in the future. The status message can also be made more generic to report which OS was detected.
# Check for unsupported OS for LTO
if((OS_ID STREQUAL "debian" AND OS_VERSION_ID STREQUAL "11") OR
(OS_ID STREQUAL "rocky" AND OS_VERSION_ID MATCHES "^8"))
set(LTO_UNSUPPORTED_OS TRUE)
message(STATUS "Detected unsupported OS (${OS_ID} ${OS_VERSION_ID}): LTO will be disabled due to known build issues")
endif()
| if(UPPER_BUILD_TYPE STREQUAL "RELEASE") | ||
| set(LTO_STATUS "enabled") | ||
| endif() |
There was a problem hiding this comment.
The status message for LTO can be misleading. When the build type is not RELEASE, it reports LTO as "disabled", which is true for the current build but doesn't convey that LTO is configured and will be used for RELEASE builds. This could cause confusion. It would be more informative to indicate that LTO is configured for Release builds even when the current build type is different.
if(UPPER_BUILD_TYPE STREQUAL "RELEASE")
set(LTO_STATUS "enabled")
else()
set(LTO_STATUS "configured for Release builds")
endif()
Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Please run a secure CI to ensure we don't have any issues. @gadfort FYI if you want to test it. |
|
@maliberty mine just finished (I started it earlier when I saw the PR): https://github.com/siliconcompiler/scgallery/actions/runs/21002832081 Looks to be passing for me
|
|
I see a secure CI run was done already and passed. |
4 participants
gui::save_imagecall. It was because LTO is mistakenly enabled for GUI static library in CMake env. This PR resolves the issue.