build: Enable LTO on-by-default for CMake and LTO off-by-default for Bazel#9326
Conversation
Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Note that the default build `bazel build //:openroad` does not use LTO. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request modifies the build configurations for both CMake and Bazel to control Link-Time Optimization (LTO). For CMake, LTO is now enabled by default, which should improve performance for release builds. For Bazel, LTO is now explicitly controlled by a --config=opt flag, and a helpful notification is added to inform users when it's not being used. The changes for Bazel are well-implemented. However, I have a concern regarding the CMake change. The removed code contained a comment about LTO interfering with Qt. Enabling LTO by default might break GUI builds. I've added a specific comment with a suggestion on how to address this potential issue.
I am having trouble creating individual review comments. Click here to see my feedback.
CMakeLists.txt (24-29)
The removed code block included a comment stating that LTO interferes with Qt (# Interfers with Qt so off by default.). By enabling LTO by default, this could potentially break GUI builds which use Qt.
Has this interference been resolved? If not, it would be safer to conditionally disable LTO when BUILD_GUI is enabled to avoid breaking existing functionality. For example, you could add logic to disable LINK_TIME_OPTIMIZATION if BUILD_GUI is true.
Yes. LTO is disabled for GUI module. |
CMake
Bazel
--config=opt. (e.g.,bazel build //:openroad --config=opt).-c optand disable LTO for all other compile modes (e.g.,-c fastbuild,-c dbg, etc) requires modification of allBUILDfiles.BUILDfiles, implemented the LTO-on under--config=optoption.--config=optis not given.Issue
Fixes #9050
Previous PRs
#9254
#9069