target_features: sse (or at least avx2) is incompatible with soft-float ABI

[RalfJung]

View all comments

Fixes #117938

Enabling both the avx2 and soft-float target features is not supported by LLVM and can crash the backend. Let's preempt that with rust-level checks. (I still think there's also an LLVM bug here, it shouldn't just SIGILL on unexpected target feature configurations, but that's a different discussion.)

What is not clear to me is whether this just affects just avx2 or also avx or even sse (we don't support mmx/3dnow separately). @dianqk do you know more about this? To be safe, let's reject "sse" and therefore by implication also all other x86 vector target features.

This PR turns #[target_feature(enable = "sse")] on a softfloat target into an FCW similar to what we do on aarch64 (see #135160). The FCW only affects people building for soft-float targets which is a fairly small percentage of our overall users (and which means we cannot meaningfully crater this). I hence went for "report in deps" immediately so that the people building the actual binaries see these warnings that their upstreams probably will never see.

[rustbot]

r? @khyperia

rustbot has assigned @khyperia.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 16 candidates

[RalfJung]

r? @workingjubilee or @dianqk

[rustbot]

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

[RalfJung]

Hm unfortunately it seems like LLVM does not crash on all functions with #[target_feature(enable = "avx2")]. This one for example works fine:

#[unsafe(no_mangle)]
#[target_feature(enable = "avx2")]
pub fn foobar(x: __m256i, y: __m256i) -> __m256i {
    _mm256_or_si256(x, y)
}

So we may have to add an FCW for this after all.

@tarcieri do you know which function is causing the trouble in dalek-cryptography/curve25519-dalek#601? All we know is that it's somewhere in poly1305...

[RalfJung]

Okay I have a reproducer:

#![no_std]

use core::arch::x86_64::*;

#[unsafe(no_mangle)]
#[target_feature(enable = "avx2")]
pub fn foobar(ptr: *const __m256i) -> __m256i { unsafe {
    let key = _mm256_loadu_si256(ptr);
    _mm256_and_si256(
        _mm256_permutevar8x32_epi32(key, _mm256_set_epi32(3, 7, 2, 6, 1, 5, 0, 4)),
        _mm256_set_epi32(0, -1, 0, -1, 0, -1, 0, -1),
    )
}}

[dianqk]

I don't know these features on x86, but the PR seems reasonable to me.

[RalfJung]

With Nikita on vacation, who might know which features LLVM supports on x86 in combination with +soft-float?

But I guess we can also just warn about all vector features (as this PR does now) and if we get issues saying sse actually works fine we can always adjust. 🤷

[RalfJung]

FWIW the s390x target also has a "soft-float" target feature and there we already mark "vector" as incompatible.

ARM also has "soft-float" and there we don't mark anything. ARM also has much more explicit ABI control so maybe setting FloatABIType to "soft" and enabling neon actually works fine there? No idea.

[traviscross]

Makes sense to me. Thanks @RalfJung.

@rfcbot fcp merge lang

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[joshtriplett]

This seems fine, as long as we only reject an explicit request for the combination of these features, but don't reject the use of soft-float on x86-64 targets where the baseline has SSE/SSE2 available.

(If that case also causes breakage, that'd be more urgent to fix since it'd prevent all usage of soft-float on x86-64.)

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[RalfJung]

@joshtriplett

but don't reject the use of soft-float on x86-64 targets where the baseline has SSE/SSE2 available.

I am not sure what you mean by that... but if you mean -Ctarget-feature=+soft-float on e.g. x86_64-unknown-linux-gnu then that already emits a warning (saying it will be a hard error in the future) because it is unsound. It alters how floats are returned. This PR doesn't change anything for x86_64-unknown-linux-gnu and similar targets though, so that is a separate discussion.

[RalfJung]

@workingjubilee reminder that this is waiting for review (in parallel with FCP). :)
Or should I roll a different reviewer?

[workingjubilee]

Oh yes. I actually had started taking a look earlier today, just had gotten interrupted.

[workingjubilee]

Code looks fine, minor note on an oddity I thought of which hopefully won't require us to redraft anything except either filing a new issue or maybe we decide to broaden lint scope slightly if we think it's same-in-principle.

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[RalfJung]

@bors r=workingjubilee rollup

[rust-bors]

📌 Commit 2c87740 has been approved by workingjubilee

It is now in the queue for this repository.