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.

[RalfJung]

Ah, hm, this is tricky... stdarch no longer builds on x86_64-unknown-none because it enables target features that must not be enabled on that target.

For now we can allow the lint. But we'll need something better long-term, something like #160301.

[rustbot]

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

[rustbot]

portable-simd is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

[RalfJung]

@bors try jobs=dist-various*
@bors rollup=iffy

[rust-bors]

☀️ Try build successful (CI)
Build commit: 58729dd (58729dd2855f18195fd476b08899b00a57ff537c)
Base parent: be0ea33 (be0ea33bba5000ac134e58acde780af83d908e44)

[RalfJung]

@bors r=workingjubilee

[rust-bors]

📌 Commit 68c66e8 has been approved by workingjubilee

It is now in the queue for this repository.

[rust-bors]

⌛ Testing commit 68c66e8 with merge 5125805...

Workflow: https://github.com/rust-lang/rust/actions/runs/32647296368

[JonathanBrouwer]

@bors yield
Yielding to enclosing rollup

[rust-bors]

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/32647296368

The next pull request likely to be tested is #161605.

[RalfJung]

You're brave, rolling this up a 2nd time.^^

[JonathanBrouwer]

My idea there is that if the rollup fails because of this PR, we don't waste any queue time because this PR would've failed regardless and used the same amount of time

[rust-bors]

⌛ Testing commit 68c66e8 with merge 2524af0...

Workflow: https://github.com/rust-lang/rust/actions/runs/32650693547

[JonathanBrouwer]

@bors yield

[rust-bors]

Auto build was cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/32650693547

The next pull request likely to be tested is #161605.