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

Pick MSVC -arch: based on CARGO_CFG_TARGET_FEATURE for x86 / x86_64 targets #713

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wildbook
Copy link

@wildbook wildbook commented Aug 7, 2022

This is my first contribution to cc-rs, and I'm more than open to any critique or questions.


As it is now, -arch:IA32 is passed for 32-bit builds if the target is i586 or clang-cl is being used. This explicit override often breaks assumptions and always downgrades 32-bit clang-cl builds from the default -arch:SSE2 to -arch:IA32, potentially impacting performance.

This PR attempts to only downgrade when there's reason to, and otherwise attempt to pick a target -arch that matches the features set in CARGO_CFG_TARGET_FEATURE. Picking an -arch that follows CARGO_CFG_TARGET_FEATURE also means crates (both x86 and x86_64) that opt into support for AVX or AVX2 through target-feature will now also have any dependencies built with cc benefit from that.


I'm using clang-cl to build a few 32-bit projects, so I'm most likely running into issues with this far more often than most people are. While usually it's at most a bit annoying that cc builds aren't respecting target-feature, it actually breaks my builds.

Today I ran into this same issue again when trying to integrate mimalloc_rust, which requires SSE2 for atomics.

Since this is the third crate I've attempted to use that has issues due to the same forced override, I decided to try fix the problem in cc-rs instead of forking yet another crate just to add .flag_if_supported("-arch:SSE2") to be able to build it successfully.

I opted to not add support for AVX512 for now since MSVC bundles all AVX512 extensions under the same -arch:AVX512, while Rust treats each extension as a separate feature. Adding support for it is possible, but I don't need it myself and I don't know how to do it cleanly, so I'll leave that to someone else. For now it should fallback to AVX2, which is still an upgrade over the previous IA32 (clang-cl) / SSE2 (cl.exe).

It's also possible to do the same thing for ARM builds, but since I have no experience with ARM I'll leave that task too for someone else.

@ChrisDenton
Copy link
Contributor

Hm, I was under the impression that I586 was intended explicitly for older CPUs that do not have newer instructions so is this not working as intended? Otherwise, how is it different from i686?

@wildbook
Copy link
Author

wildbook commented Aug 7, 2022

It is, but the forced downgrade for clang-cl is not currently checking for i586. It's unconditionally passing -arch:IA32 for any arch containing 86 that doesn't also contain x86_64, which includes i686.

For "normal" MSVC it's already working "as intended", it's just that "as intended" in this case means "assume the target supports SSE2 and nothing else" (MSVC's default) for i686 / x86_64. Which is completely fine, but means that even if your target supports AVX instructions and you've opted into them through rustc's target-feature MSVC will not use them.

I had issues due to the former part, and implemented the latter while I was at it since I was already working on making it slightly smarter.

@thomcc thomcc added the O-windows Windows targets and toolchains label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Windows targets and toolchains
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants