is_x86_feature_detected!("avx512f") fails to build on beta and nightly

[oconnor663]

The following program builds and runs on stable but fails to build on nightly:

fn main() {
    dbg!(is_x86_feature_detected!("avx512f"));
}

Here's the build error on rustc 1.43.0-nightly (58b834344 2020-02-05):

error[E0658]: use of unstable library feature 'stdsimd'
 --> src/main.rs:2:10
  |
2 |     dbg!(is_x86_feature_detected!("avx512f"));
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/27731
  = help: add `#![feature(stdsimd)]` to the crate attributes to enable
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

But note that if I replace "avx512f" with "avx2", the error goes away.

[Mark-Simulacrum]

cc @gnzlbg

[oconnor663]

Here's the output of cargo-bisect-rustc:

searched nightlies: from nightly-2020-01-02 to nightly-2020-02-10
regressed nightly: nightly-2020-02-06
searched commits: from c9290dc to 58b8343
regressed commit: eda1a7a

That points to #68755, which is a large merge of stdarch. When I bisect stdarch, I find the error was introduced in rust-lang/stdarch@cb6fa68, possibly intentionally. Is it the case that AVX-512 feature detection was never intended to be stabilized? Or maybe it got caught up in a larger group of unstable features? Prior to this regression, it did in fact work :)

[oconnor663]

A quick search through https://github.com/search?q=is_x86_feature_detected%21%28%22avx512f%22%29&type=Code turns up at least one other crate besides BLAKE3 that fails to build on nightly for this reason:

git clone https://github.com/PoC-Consortium/scavenger
cd scavenger
cargo check --features=simd           # succeeds
cargo +nightly check --features=simd  # fails

[oconnor663]

@Mark-Simulacrum, this might be a hi-pri regression. (Though of course I'm biased cause it affects my crate.) If @gnzlbg isn't available to evaluate it, could we pull in someone else?

[Mark-Simulacrum]

cc @rust-lang/libs

To be honest I'm not even entirely sure where this macro is implemented...

[Amanieu]

Caused by rust-lang/stdarch#739, which started enforcing stability on individual features.

[oconnor663]

It looks like a crater run was done in September, but the blake3 and b3sum crates were published in January. The blake3 crate relies on std to detect AVX-512 support, but the actual AVX-512 implementation code is in C or assembly. Also the scavenger crate I mentioned above hides its use of AVX-512 detection behind its simd feature, so I don't think crater would've caught it.

[Mark-Simulacrum]

Seems like there was no discussion done of whether this would break folks (or at least I can't find it). Clearly this is "permitted" under a strict interpretation of our stability rules, though is not documented on the macro at all; the docs list avx512f with no warning that it is unstable. That at least sounds like a bug.

I guess someone (libs team?) needs to make a call on whether this is a bug, and whether we should fix it.

[Amanieu]

Considering that this broke two crates on stable, I think we should just go ahead and stabilize the avx512 features. I don't think there is any particular reason for holding back on them.

[pnkfelix]

(nominating to try to ensure T-libs resolves this in some manner before this hits stable.)

[sfackler]

Even if we don't want to stabilize the intrinsics themselves for whatever reason, it seems fine to allow detection of avx512 support via the macro.

[Mark-Simulacrum]

Could someone on T-libs fcp a stabilization for the following x86 features, which have been destabilized in nightly:

• mmx
• sse4a
• avx512f
• avx512cd
• avx512er
• avx512pf
• avx512bw
• avx512dq
• avx512vl
• avx512ifma
• avx512vbmi
• avx512vpopcntdq
• tbm

https://doc.rust-lang.org/nightly/std/macro.is_x86_feature_detected.html#supported-arguments

Once we get the ball rolling on that, I can work on trying to fix whatever the root cause of this destabilization is.

[Mark-Simulacrum]

(Or, if we don't want to stabilize all of them, and want to say "oh well breaking change", then that would be helpful too).

[Amanieu]

Also see #60109: there was an approved FCP to stabilize adx, tbm and sse4a but it got closed because the PR author was inactive.

[Amanieu]

According to rust-lang/stdarch#310, it seems that a lot of AVX512 intrinsics are missing from std::arch. As such I don't think it really makes sense to stabilize the AVX512 feature.

I think the best approach would be to stabilize avx512 only for is_x86_feature_detected, since it was already (accidentally) stable and code is relying on it.

Any thoughts @rust-lang/libs @gnzlbg? I will write up a PR for stdarch if there are no objections.

[BurntSushi]

It seems to me that stabilizing avx512 just as a parameter to is_x86_feature_detected should be fine? As in, I don't think there's any hazard of us never stabilizing it.

I'd be more skeptical of blanket stabilizing a bunch of parameters to is_x86_feature_detected though. For example, we might want to never stabilize anything to do with mmx.

it seems that a lot of AVX512 intrinsics are missing from std::arch. As such I don't think it really makes sense to stabilize the AVX512 feature.

Definitely agree here.

[pnkfelix]

triage: After spending a while explaining this bug to @spastorino , I figured I should take the plunge and mark this as P-high.

(I'm not 100% sure if it is a release blocker per se, but my interpretation of the comments above is that we would prefer to either land a fix for this before beta is cut or, failing that, prioritize a backport to the beta branch, so that the bug itself never hits stable.)

[Amanieu]

OK, let's try to make some progress on this. I'd like to propose stabilizing the following features for feature detection only:

• mmx
• sse4a
• tbm
• avx512f
• avx512cd
• avx512er
• avx512pf
• avx512bw
• avx512dq
• avx512vl

The SSE4a and TBM features were already scheduled to be stabilized in #60109, but the PR got abandoned. Only a subset of the AVX512 features are listed here because there is some uncertainty with the newer ones as to whether they should contain an underscore: avx512vbmi vs avx512_vbmi. Finally MMX, while not supported by LLVM, is a well known feature name.

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @Centril
• @KodrAus
• @SimonSapin
• @cramertj
• @dtolnay
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @sfackler
• @withoutboats

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!

See this document for info about what commands tagged team members can give me.

[oconnor663]

rust-lang/stdarch#842 is now up to speed with @Amanieu's proposal above.

[rfcbot]

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

[oconnor663]

As of the 1.42 release today, cargo +beta install b3sum is failing. This fix will need to be backported.

[spastorino]

@rustbot assign @Amanieu

[Amanieu]

PR to update the submodule is here: #70151

It will need to be backported to beta.