Stabilize x86/x86_64 intrinsics #414

alexcrichton · 2018-04-04T19:38:11Z

This commit stabilizes all intrinsics in the x86 and x86_64 modules, namely
allowing stabilization of the arch::x86 and arch::x86_64 module in libstd.
Stabilizations here were applied in an automated fashion using this
script, and notably everything related to __m64 was omitted from this
round of stabilization

gnzlbg · 2018-04-04T19:53:30Z

coresimd/x86/cpuid.rs

@@ -44,8 +49,11 @@ pub struct CpuidResult {
 /// [wiki_cpuid]: https://en.wikipedia.org/wiki/CPUID
 /// [intel64_ref]: http://www.intel.de/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-instruction-set-reference-manual-325383.pdf
 /// [amd64_ref]: http://support.amd.com/TechDocs/24594.pdf
+///
+/// [Intel's documentation](https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=__cpuid_count)


This is not an "official" Intel intrinsic, so this link returns empty. IIRC at least the cpuid intrinsics and the read/writeeflags intrinsics will suffer from this. There should be a list of these or their modules in the stdsimd-verify module.

gnzlbg · 2018-04-04T19:54:52Z

coresimd/x86/mod.rs

@@ -334,6 +340,7 @@ pub use self::test::*;

 #[doc(hidden)]
 #[allow(non_camel_case_types)]
+#[stable(feature = "simd_x86", since = "1.27.0")]
 pub(crate) trait m128iExt: Sized {


I don't recall exactly what pub(crate) does so am just tripple checking this: these traits should not be exported by core right?

gnzlbg · 2018-04-04T19:55:11Z

coresimd/x86/mod.rs

@@ -598,5 +606,3 @@ pub use self::aes::*;
 mod rdrand;
 pub use self::rdrand::*;

-mod sha;


What happened here?

gnzlbg · 2018-04-04T19:56:52Z

coresimd/x86/sha.rs

@@ -26,29 +26,38 @@ use stdsimd_test::assert_instr;
 /// Perform an intermediate calculation for the next four SHA1 message values


Looks like this module was removed from x86/mod.rs :/

gnzlbg · 2018-04-04T20:02:30Z

coresimd/x86/sse4a.rs

@@ -33,9 +33,12 @@ extern "C" {
 ///
 /// If `length == 0 && index > 0` or `lenght + index > 64` the result is
 /// undefined.
+///
+/// [Intel's documentation](https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm_extract_si64)


So these intrinsics are part of the AMD intrinsics, and are not in the intel IntrinsicsGuide. AMD only provides a pdf with the spec AFAICT so the correct place to point them at would be:

/// [AMD64 Architecture Programmer's Manual, Volume 3: General-Purpose and System Instructions](http://support.amd.com/TechDocs/24594.pdf)

gnzlbg · 2018-04-04T20:03:14Z

coresimd/x86/tbm.rs

@@ -67,62 +71,80 @@ pub fn _bextr2_u64(a: u64, control: u64) -> u64 {
 /// Clears all bits below the least significant zero bit of `x`.
 ///
 /// If there is no zero bit in `x`, it returns zero.
+///
+/// [Intel's documentation](https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_blcfill_u32)
 #[inline]


here the same as for sse4a applies, these are AMD only.

gnzlbg · 2018-04-04T20:03:53Z

coresimd/x86/tbm.rs

@@ -37,6 +38,7 @@ pub fn _bextr_u32(a: u32, start: u32, len: u32) -> u32 {
 /// the least significant bits of the result.
 #[inline]
 #[target_feature(enable = "tbm")]
+#[stable(feature = "simd_x86", since = "1.27.0")]


I think it would be better to remove this in this comment. Not that we fix it some day and it suddenly becomes stable.

gnzlbg · 2018-04-04T20:04:33Z

coresimd/x86/test.rs

@@ -12,6 +12,7 @@ pub unsafe fn assert_eq_m64(a: __m64, b: __m64) {
 }

 #[target_feature(enable = "sse2")]
+#[stable(feature = "simd_x86", since = "1.27.0")]


I don't think anything in this file should have this attribute.

gnzlbg · 2018-04-04T20:05:57Z

coresimd/x86_64/bmi.rs

@@ -14,10 +14,13 @@ use stdsimd_test::assert_instr;



OCD: I just realized that this file is still called bmi.rs, but this and the one in x86 should be renamed to bmi1.rs to match the target feature name.

gnzlbg · 2018-04-04T20:06:38Z

coresimd/x86_64/mod.rs

@@ -38,5 +38,3 @@ pub use self::avx2::*;
 mod bswap;
 pub use self::bswap::*;

-mod rdrand;


Here again, was this intended?

gnzlbg · 2018-04-04T20:08:12Z

stdsimd/arch/detect/arch/x86.rs

+/// This macro only takes one argument which is a string literal of the feature
+/// being tested for. The feature names supported are the lowercase versions of
+/// the ones defined by Intel in [their documentation][docs].
+///


Do we want to list the whitelisted features here?

Maybe also mentions that these names are the same as those used by #[target_feature], and maybe it might be also worth mentioning twice that is_x86_feature_detected("avx,avx2") is not supported (even though it works for target feature.

Excellent ideas!

gnzlbg · 2018-04-04T20:12:49Z

So the first pass looks good! There are a couple of places where the script needs manual intervention, once those are finished and test pass I'll go over this one more time in more detail.

alexcrichton · 2018-04-04T22:34:51Z

Thanks for the close eye on review! I think I've addressed everything

alexcrichton · 2018-04-04T22:50:13Z

Also FWIW we should hold off on merging this until the FCP has finished

gnzlbg · 2018-04-06T13:21:30Z

Looks good!

This commit stabilizes all intrinsics in the `x86` and `x86_64` modules, namely allowing stabilization of the `arch::x86` and `arch::x86_64` module in libstd. Stabilizations here were applied in an automated fashion using [this script][scr], and notably everything related to `__m64` was omitted from this round of stabilization [scr]: https://gist.github.com/alexcrichton/5b456d495d6fe1df46a158754565c7a5

alexcrichton · 2018-04-13T14:14:11Z

Ok! I've merged this into a next-breaking branch and we'll update to that in the Rust repo. Once it's landed in nightly I'll circle back here to fix CI.

alexcrichton · 2018-04-13T14:32:19Z

Er actually looks like it all pased! (except rustfmt)

gnzlbg · 2018-04-13T14:35:40Z

Yay!! Congrats!

This commit stabilizes the SIMD in Rust for the x86/x86_64 platforms. Notably this commit is stabilizing: * The `std::arch::{x86, x86_64}` modules and the intrinsics contained inside. * The `is_x86_feature_detected!` macro in the standard library * The `#[target_feature(enable = "...")]` attribute * The `#[cfg(target_feature = "...")]` matcher Stabilization of the module and intrinsics were primarily done in rust-lang/stdarch#414 and the two attribute stabilizations are done in this commit. The standard library is also tweaked a bit with the new way that stdsimd is integrated. Note that other architectures like `std::arch::arm` are not stabilized as part of this commit, they will likely stabilize in the future after they've been implemented and fleshed out. Similarly the `std::simd` module is also not being stabilized in this commit, only `std::arch`. Finally, nothing related to `__m64` is stabilized in this commit either (MMX), only SSE and up types and intrinsics are stabilized. Closes rust-lang#29717 Closes rust-lang#44839 Closes rust-lang#48556

Stabilize x86/x86_64 SIMD This commit stabilizes the SIMD in Rust for the x86/x86_64 platforms. Notably this commit is stabilizing: * The `std::arch::{x86, x86_64}` modules and the intrinsics contained inside. * The `is_x86_feature_detected!` macro in the standard library * The `#[target_feature(enable = "...")]` attribute * The `#[cfg(target_feature = "...")]` matcher Stabilization of the module and intrinsics were primarily done in rust-lang/stdarch#414 and the two attribute stabilizations are done in this commit. The standard library is also tweaked a bit with the new way that stdsimd is integrated. Note that other architectures like `std::arch::arm` are not stabilized as part of this commit, they will likely stabilize in the future after they've been implemented and fleshed out. Similarly the `std::simd` module is also not being stabilized in this commit, only `std::arch`. Finally, nothing related to `__m64` is stabilized in this commit either (MMX), only SSE and up types and intrinsics are stabilized. Closes #29717 Closes #44839 Closes #48556

alexcrichton requested a review from gnzlbg April 4, 2018 19:38

alexcrichton mentioned this pull request Apr 4, 2018

Stabilize x86/x86_64 SIMD rust-lang/rust#49664

Merged

gnzlbg reviewed Apr 4, 2018

View reviewed changes

alexcrichton force-pushed the stabler branch from 4038d91 to 7fd038b Compare April 4, 2018 22:34

alexcrichton force-pushed the stabler branch from 7fd038b to 01ed2bb Compare April 5, 2018 14:02

alexcrichton force-pushed the stabler branch from 01ed2bb to c970010 Compare April 6, 2018 21:27

alexcrichton force-pushed the stabler branch from c970010 to e1ba331 Compare April 9, 2018 22:12

alexcrichton force-pushed the stabler branch from e1ba331 to 01ed2bb Compare April 13, 2018 14:12

alexcrichton force-pushed the stabler branch from 01ed2bb to 01d7fd9 Compare April 13, 2018 14:13

alexcrichton merged commit 2397665 into rust-lang:master Apr 13, 2018

alexcrichton deleted the stabler branch April 13, 2018 14:32

RalfJung mentioned this pull request Aug 8, 2023

Non-temporal stores (and _mm_stream operations in stdarch) break our memory model rust-lang/rust#114582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize x86/x86_64 intrinsics #414

Stabilize x86/x86_64 intrinsics #414

alexcrichton commented Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

alexcrichton Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

gnzlbg Apr 4, 2018

alexcrichton Apr 4, 2018

gnzlbg commented Apr 4, 2018

alexcrichton commented Apr 4, 2018

alexcrichton commented Apr 4, 2018

gnzlbg commented Apr 6, 2018

alexcrichton commented Apr 13, 2018

alexcrichton commented Apr 13, 2018

gnzlbg commented Apr 13, 2018

		@@ -26,29 +26,38 @@ use stdsimd_test::assert_instr;
		/// Perform an intermediate calculation for the next four SHA1 message values

Stabilize x86/x86_64 intrinsics #414

Stabilize x86/x86_64 intrinsics #414

Conversation

alexcrichton commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg commented Apr 4, 2018

alexcrichton commented Apr 4, 2018

alexcrichton commented Apr 4, 2018

gnzlbg commented Apr 6, 2018

alexcrichton commented Apr 13, 2018

alexcrichton commented Apr 13, 2018

gnzlbg commented Apr 13, 2018