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

[bindings] Add LTO builds on Windows+MSVC #1687

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

nicholasjng
Copy link
Contributor

Gates the MSVC switches behind an @bazel_skylib:selects statement.

This is a first experiment from best guesses and studying the MSVC docs.

Gates the MSVC switches behind an `@bazel_skylib:selects` statement.

This is a first experiment from best guesses and studying the MSVC docs.
@nicholasjng
Copy link
Contributor Author

We should compare the perf numbers from the Python test for this build to those of some of the previous jobs - I read a forum post today about how MSVC LTO can in some cases tank performance.

@@ -33,6 +44,7 @@ cc_library(
"include",
],
linkopts = select({
":winplusmsvc": ["/LTGC"], # allows other compilers on Windows than just MSVC.
Copy link
Member

Choose a reason for hiding this comment

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

so clang on windows takes /LTGC as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I guess that came from the comment. The :winplusmsvc condition only applies for Windows + MSVC, so for Windows + clang, it falls through to //conditions:default, which is no options.

I can update the comment if it's too misleading.

@LebedevRI LebedevRI changed the title Add LTO builds on Windows+MSVC [bindings] Add LTO builds on Windows+MSVC Oct 26, 2023
@dmah42 dmah42 merged commit b219e18 into google:main Oct 27, 2023
54 of 60 checks passed
@nicholasjng nicholasjng deleted the reapply-buildopts branch April 15, 2024 16:46
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.

2 participants