module-template: fix the IPO check - #13
Merged
Merged
Conversation
The IPO support check could never pass on macOS, and the failure was silent. moderngekko-port configures with CMAKE_NINJA_FORCE_RESPONSE_FILE=1, which the module genuinely needs -- 186 objects do not fit on a command line. check_ipo_supported's try-compile inherits the environment, so its static library step runs `ar qc libfoo.a @foo.rsp`, and Apple's /usr/bin/ar has no @file support: ar: @CMakeFiles/foo.rsp: No such file or directory The check therefore failed, CMake logged one "Module IPO disabled:" line, and the build carried on without -flto=thin -- while manifest.txt went on recording `flags=compile:-O2 -flto=thin ... link:-flto=thin`. Object files were plain Mach-O rather than LLVM bitcode. No module built on macOS has ever had cross-translation-unit inlining, which is worth knowing before drawing any conclusion about why a runtime helper did not get inlined into generated code. Scope the variable off around the check only and restore it before the generator reads it, so the real link keeps its response files. Also flip RECOMPCORE_MODULE_ENABLE_IPO to OFF, because with the check repaired LTO was measured and did not pay. Mario Kart: Double Dash on arm64, same PGO profile, only -flto=thin differing, alternating A/B with a reversed final block on one savestate: no LTO n=5 mean 1.1826 [1.1729-1.1959] LTO n=5 mean 1.1733 [1.1387-1.1879] -0.8%, ranges overlap -- unproven The host was not quiet, so this is "no win" rather than "a loss". Pushing further -- -Wl,-mllvm,-import-instr-limit=500 -Wl,-mllvm,-inline-threshold=600, which does inline the hot FP helper, taking ppc_fmuls from 5846 call sites to 861 -- measured -1.7% and grew the module by 7 MB. One caveat on the default. On Linux, GNU ar does support @file, so the check presumably passed there and those builds have had thin-LTO all along; OFF takes it away from them. That is not backed by a Linux measurement. If you would rather not change Linux, take the check fix and drop the default change -- they are independent, and the option can still be set per build.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replacement for #11 because its head repository is a sibling fork and GitHub does not permit maintainer pushes to it.
Includes the response-file IPO check fix while preserving the existing default of enabling IPO when supported.
Validation: configured the module template with Clang 19 and forced Ninja response files; ThinLTO remained enabled.