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

Add compiler flags -Os and -Oz to optimize binary size #14463

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Apr 9, 2024

Follows LLVM defaults. Tajkes advantage that LLVMRunPasses now handles Os and Oz directly, and manually applies the LLVM defaults for LLVM 13 to 16 (new pas manager). The Oz configuration with the legacy manager (LLVM < 13) don't disable the vectorizations; I'm not sure how to do it or if it's worth implementing.

Maybe we'd like to adjust the inliner thresholds to account for Crystal. For example Rust uses 75 and 25. Clang also keeps the SLP vectorization enabled. I didn't check the impact (yet).

Of course this is related to #14393

ysbaddaden and others added 3 commits April 9, 2024 11:35
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
They could be confusing: they're not opt level 4 or 5 but opt size and opt mini size.
src/llvm/pass_builder_options.cr Outdated Show resolved Hide resolved
ysbaddaden and others added 2 commits April 9, 2024 11:50
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The man page also needs an update.

man/crystal.1 Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor Author

LLVM only added LLVMPassBuilderOptionsSetInlinerThreshold in LLVM 17 😮‍💨

llvm/llvm-project@c20a9bb

@ysbaddaden
Copy link
Contributor Author

I backported the function. It relies on a C++ class (for unwrap) that is defined in a CPP source file, not in a distributed header file. Luckily the class looks identical between LLVM 13 and 16. That's still kinda ugly to have to copy-paste it 😞

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 12, 2024

Wonderful, now it looks like the backport is only compatible with LLVM 16 :feelsgood:

llvm/llvm-project@4fa3280

@straight-shoota
Copy link
Member

Perhaps we could limit support for these new option to LLVM 17+?

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 12, 2024

Or keep the legacy pass manager for LLVM < 17? or LLVM < 16 with the backported function.

@straight-shoota straight-shoota removed this from the 1.13.0 milestone Apr 17, 2024
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 20, 2024

I went with a mix of PassManager (legacy) and PassBuilder. While older releases (LLVM <= 12) always use PassManager, and the newest releases (LLVM >= 17) always use PassBuilder, the intermediary releases (LLVM 13 to 16) preferably use PassBuilder but fallback to use the legacy PassManager for Os and Oz.

Hopefully, that should now compile, and avoid to segment the opt levels to the LLVM version that Crystal happens to have been compiled with (all versions from LLVM 12 to 18 compiled on my local) 🤞

Of course it doesn't compile with LLVM 10 on CI 😮‍💨 but it compiles on my local :feelsgood: It was because older crystal releases don't support has_method? and/or has_constant? macro methods.

Don't remove the types and only up the deprecation to only warn from
LLVM 17, even though the feature was removed, in order to keep the
documentation, while avoiding a deprecation warning when compiling
crystal on LLVM 13 to 16.
@straight-shoota straight-shoota added this to the 1.13.0 milestone Apr 22, 2024
@straight-shoota straight-shoota changed the title Add -Os and -Oz to optimize for size rather than speed Add compiler flags -Os and -Oz to optimize binary size Apr 24, 2024
@straight-shoota straight-shoota merged commit dcccbc5 into crystal-lang:master Apr 24, 2024
60 checks passed
@ysbaddaden ysbaddaden deleted the feature/optimize-for-size-rather-than-speed branch April 27, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants