Skip to content
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

ci: Use link-time optimization for building #1636

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

MajorP93
Copy link
Contributor

@MajorP93 MajorP93 commented Dec 1, 2024

This PR enables LTO if the respective build toolchain supports it.

During my tests LTO enabled builds of shadPS4 performed noticeably better in certain games (e.g. Bloodborne).
Especially 1% low FPS measurement results were better and perceived stutter was less.

This observation was backed by other testers that were given a test build by me.

@baggins183
Copy link
Contributor

This shouldnt apply to debug builds

@baggins183
Copy link
Contributor

IMO it would be good to wrap this in some variable check so its off by default for anyone building locally but can be enabled by -Dwhatever for github builds

@abouvier
Copy link
Contributor

abouvier commented Dec 1, 2024

This is a user preference, you should just pass -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON to the cmake call if you want (if you compiler supports it).

@ngoquang2708
Copy link
Contributor

So this config it better set in GitHub CI instead?

@abouvier
Copy link
Contributor

abouvier commented Dec 1, 2024

So this config it better set in GitHub CI instead?

Yes, like every other build preferences (compiler, optimization level, etc.), or in a CMakePresets.json file.

@MajorP93 MajorP93 changed the title cmake: Enable LTO if compiler in build environment supports it ci: Enable LTO for build type "Release" Dec 2, 2024
@MajorP93 MajorP93 changed the title ci: Enable LTO for build type "Release" ci: Use link-time optimization for building Dec 2, 2024
@ngoquang2708
Copy link
Contributor

Missing CMAKE_ prefix.

@MajorP93
Copy link
Contributor Author

MajorP93 commented Dec 2, 2024

Missing CMAKE_ prefix.

Thanks for the thint.

Updated the PR: following your recommendations I enabled LTO in the GH action instead of CMakeLists.txt in order to leave the option of building with having it disabled for people compiling the project locally.

@abouvier
Copy link
Contributor

abouvier commented Dec 3, 2024

You can put set(CMAKE_POLICY_DEFAULT_CMP0069 NEW) at the top of externals/CMakeLists.txt to configure third-party projects. It won't do anything when LTO is disabled.

@MajorP93 MajorP93 force-pushed the lto branch 3 times, most recently from 6441f90 to 42d9acc Compare December 4, 2024 12:49
@MajorP93
Copy link
Contributor Author

MajorP93 commented Dec 4, 2024

@abouvier thanks for the hint.
I updated the PR accordingly.
I used cmake_policy(SET CMP0069 NEW) instead of set(CMAKE_POLICY_DEFAULT_CMP0069 NEW) as this seems to be the recommended way of setting the policy in a CMakeLists.txt file.

Compiled locally, behaves as expected.

@abouvier
Copy link
Contributor

abouvier commented Dec 4, 2024

cmake_policy is for setting the policies of your own project, not the externals ones. The policy CMP0069 is already set to NEW for shadPS4 because of the cmake_minimum_required(VERSION 3.24) at the very beginning.

See https://cmake.org/cmake/help/latest/variable/CMAKE_POLICY_DEFAULT_CMPNNNN.html

@MajorP93
Copy link
Contributor Author

MajorP93 commented Dec 4, 2024

cmake_policy is for setting the policies of your own project, not the externals ones. The policy CMP0069 is already set to NEW for shadPS4 because of the cmake_minimum_required(VERSION 3.24) at the very beginning.

See https://cmake.org/cmake/help/latest/variable/CMAKE_POLICY_DEFAULT_CMPNNNN.html

I read some documentation and was able to verify that CMAKE_POLICY_DEFAULT_CMPNNNN makes more sense here.

Updated accordingly. Thanks.

@baggins183
Copy link
Contributor

should this use -DCMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE instead?
https://cmake.org/cmake/help/latest/prop_tgt/INTERPROCEDURAL_OPTIMIZATION_CONFIG.html

dont think there are non-release builds on this repo's actions but maybe good idea

* This enables LTO also when building external dependencies that do not
  handle CMP0069 in their CMake scripts.
@MajorP93
Copy link
Contributor Author

MajorP93 commented Dec 6, 2024

should this use -DCMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE instead? https://cmake.org/cmake/help/latest/prop_tgt/INTERPROCEDURAL_OPTIMIZATION_CONFIG.html

dont think there are non-release builds on this repo's actions but maybe good idea

I also think that the action will only be used for building "release" config but it does not hurt to only enable LTO for specifically that config.
On debug builds compiler optimization is not wanted / needed.

Thanks for the hint, I updated the PR.

ngoquang2708 added a commit to ngoquang2708/shadPS4-flatpak that referenced this pull request Dec 7, 2024
ngoquang2708 added a commit to ngoquang2708/shadPS4-flatpak that referenced this pull request Dec 8, 2024
@MajorP93
Copy link
Contributor Author

MajorP93 commented Dec 8, 2024

Anything else that needs to be addressed?
I did not encounter any issues during my tests which is why I think this PR is ready to get merged.
If there is anything that should be addressed or tested I am happy to do so.

@ZuwasieMan
Copy link

@georgemoralis may I ask about the status of this PR? Does it need further testing? Can be merged into main?
I tested a build with these compiler optimizations, and it seems like bb's fps are more stable compared to main, with less stuttering and smoother fps dips.
I think the main discussion now is to apply those optimizations only for release builds?

@MajorP93
Copy link
Contributor Author

@georgemoralis may I ask about the status of this PR? Does it need further testing? Can be merged into main? I tested a build with these compiler optimizations, and it seems like bb's fps are more stable compared to main, with less stuttering and smoother fps dips. I think the main discussion now is to apply those optimizations only for release builds?

This PR in its current form will only enable LTO for release builds (as per cmake configuration).
This makes sense as cmake debug configurations also disable other optimization flags (e.g. -O3), so in order to ensure consistency (no optimization needed for debug builds) LTO should also be disabled for these configurations.

shadPS4's CI is currently always building release configuration. LTO would always be included for CI builds as of the current state of this PR. This should be fine as CMake flag "INTERPROCEDURAL_OPTIMIZATION" will opt for Thin-LTO and never use Fat/Full-LTO. Due to this the build time does only increase a little.

@raphaelthegreat raphaelthegreat merged commit 3062799 into shadps4-emu:main Dec 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants