[coresimd] extracts the no_std components into the coresimd crate #197

gnzlbg · 2017-11-20T17:25:32Z

Extracts the no_std components into the coresimd crate, which is unconditionally no_std (most of the library).
Re-exports the coresimd crate as stdsimd + std components (e.g. run-time feature detection for ARM)
Removes the dependency on the std::os::raw::c_void type by making those intrinsics take a *mut u8 instead.

The crates coresimd and stdsimd currently are not part of the same workspace because I got linker errors. We can fix this either in this PR or a subsequent one. Also, this PR broke Windows tests. PRs to this PR that fix this are welcome.

gnzlbg · 2017-11-20T17:26:13Z

.gitignore

+/coresimd/target
+/stdsimd-test/target
+/stdsimd-test/assert-instr-macro/target
+/stdsimd-test/simd-test-macro/target


is there a convention to ignore all paths containing target? */target/* ?

Oh you can just remove the leading / on the /target line near the top of the file

gnzlbg · 2017-11-20T17:27:01Z

.travis.yml

@@ -25,14 +25,15 @@ matrix:
      script: |
        cargo install clippy
        cargo clippy --all -- -D clippy-pedantic
+        cd coresimd
+        cargo clippy --all -- -D clippy-pedantic


cargo clippy --allonly works for the crates in a workspace, butcoresimdandstdsimd` are not in the same workspace yet: rust-lang/rust-clippy#2238

cargo fmt --all applies to all crates in the sub-directory, independently of whether they share the same workspace or not. I really don't know what the intended behavior is here, but I like cargo fmt's approach.

gnzlbg · 2017-11-20T17:29:27Z

.travis.yml

  allow_failures:
    - env: RUSTFMT=On TARGET=x86_64-unknown-linux-gnu NO_ADD=1
    - env: CLIPPY=On TARGET=x86_64-unknown-linux-gnu NO_ADD=1
 install:
  - if [ "$NO_ADD" == "" ]; then rustup target add $TARGET; fi

 script:
-  - cargo generate-lockfile


cargo generate-lockfile only really works if all the crates are in a workspace, so I have added write permissions to the docker runs.

gnzlbg · 2017-11-20T17:31:05Z

Cargo.toml

 documentation = "https://docs.rs/stdsimd"
 homepage = "https://github.com/BurntSushi/stdsimd"
 repository = "https://github.com/BurntSushi/stdsimd"
 readme = "README.md"
 keywords = ["std", "simd", "intrinsics"]
-categories = ["hardware-support", "no-std"]
+categories = ["hardware-support"]


stdsimd always requires std now

gnzlbg · 2017-11-20T17:31:40Z

Cargo.toml

@@ -18,6 +18,9 @@ is-it-maintained-issue-resolution = { repository = "BurntSushi/stdsimd" }
 is-it-maintained-open-issues = { repository = "BurntSushi/stdsimd" }
 maintenance = { status = "experimental" }

+[dependencies]
+coresimd = { version = "0.0.3", path = "coresimd/" }


I've set the first version of coresimd to 0.0.3 to match that of stdsimd, we should release these together.

gnzlbg · 2017-11-20T17:34:22Z

ci/dox.sh

+  cargo clean
+  cargo build --target $target
+
+  rustdoc --target $target -o target/doc/$arch src/lib.rs --crate-name stdsimd --library-path target/$target/debug/deps


Now rustdoc needs a build of the coresimd to generate the docs for stdsimd. I haven't taken a detailed look at the docs, and haven't looked at the output of this at all. cargo doc seems to work just fine for stdsimd, wrapping the docs of both crates nicely, although some links on coresimd are still pointing to stdsimd.

gnzlbg · 2017-11-20T17:35:30Z

ci/run-docker.sh

@@ -19,7 +19,7 @@ run() {
      --env TARGET=$target \
      --env FEATURES=$2 \
      --env STDSIMD_TEST_EVERYTHING \
-      --volume `pwd`:/checkout:ro \
+      --volume `pwd`:/checkout \


I did not managed to get this to work any other way :(

The checkout directory is not readonly anymore because now building stdsimd requires building coresimd and the lock files are not generated a priori anymore.

gnzlbg · 2017-11-20T17:36:28Z

ci/run.sh

+cargo test --all --release --target $TARGET --features $FEATURES --verbose -- --nocapture
+cd ..
+cargo test --all --target $TARGET --features $FEATURES --verbose -- --nocapture
+cargo test --all --release --target $TARGET --features $FEATURES --verbose -- --nocapture


The std crate feature is gone, and so are the two $FEATURES_STD runs.

However, cargo test --all only works for workspaces, so we need to run the tests in coresimd manually...

gnzlbg · 2017-11-20T17:37:41Z

coresimd/src/lib.rs

+//! [simd_tracking_issue]: https://github.com/rust-lang/rust/issues/27731
+//! [cfg_target_feature_issue]: https://github.com/rust-lang/rust/issues/29717
+//! [simd_soundness_bug]: https://github.com/rust-lang/rust/issues/44367
+//! [target_feature_impr]: https://github.com/rust-lang/rust/issues/44839


These docs still need to be updated to reflect coresimd instead of stdsimd.

gnzlbg · 2017-11-20T17:38:33Z

coresimd/src/lib.rs

+        // x86/x86_64:
+        any(target_arch = "x86", target_arch = "x86_64")
+    )]
+    pub use runtime::{__unstable_detect_feature, __Feature};


In coresimd only x86's run-time is available, stdsimd just re-exports it.

gnzlbg · 2017-11-20T17:39:28Z

coresimd/src/runtime/bit.rs

+/// Tests the `bit` of `x`.
+pub const fn test(x: usize, bit: u32) -> bool {
+    x & (1 << bit) != 0
+}


This file is duplicated in coresimd and stdsimd...

gnzlbg · 2017-11-20T17:39:33Z

coresimd/src/runtime/cache.rs

+        CACHE.store(f(), Ordering::Relaxed);
+    }
+    bit::test(CACHE.load(Ordering::Relaxed), bit)
+}


This file is duplicated in coresimd and stdsimd...

gnzlbg · 2017-11-20T17:39:51Z

coresimd/src/runtime/macros.rs

+    fn test_macros() {
+        assert!(cfg_feature_enabled!("sse"));
+    }
+}


This file is duplicated in coresimd and stdsimd...

gnzlbg · 2017-11-20T17:40:34Z

coresimd/src/runtime/x86.rs

@@ -424,7 +423,6 @@ pub fn detect_features() -> usize {

 #[cfg(test)]
 mod tests {
-    #[cfg(feature = "std")]


std is always available when running coresimd's tests.

gnzlbg · 2017-11-20T17:42:17Z

coresimd/src/x86/i586/avx2.rs

@@ -1257,7 +1257,7 @@ pub unsafe fn _mm256_shuffle_epi8(a: u8x32, b: u8x32) -> u8x32 {
 /// # #![feature(cfg_target_feature)]
 /// # #![feature(target_feature)]
 /// #
-/// # #[macro_use] extern crate stdsimd;
+/// # #[macro_use] extern crate coresimd as stdsimd;


I was a bit lazy here because I don't know the best way to fix the examples so that they show coresimd in the docs of coresimd and stdsimd in the docs of stdsimd. So here I just changed as little as possible for now.

gnzlbg · 2017-11-20T17:45:10Z

coresimd/src/x86/i586/sse.rs

@@ -1572,7 +1571,7 @@ pub const _MM_HINT_NTA: i8 = 0;
 #[cfg_attr(test, assert_instr(prefetcht1, strategy = _MM_HINT_T1))]
 #[cfg_attr(test, assert_instr(prefetcht2, strategy = _MM_HINT_T2))]
 #[cfg_attr(test, assert_instr(prefetchnta, strategy = _MM_HINT_NTA))]
-pub unsafe fn _mm_prefetch(p: *const c_void, strategy: i8) {
+pub unsafe fn _mm_prefetch(p: *const u8, strategy: i8) {


I don't know why this intrinsic used c_void, but Intel specifies it as taking a const char*. Anyhow I did not change this to fix the API, but to remove the dependency on c_void...

The intrinsic is also untested, I've filled: #198

gnzlbg · 2017-11-20T17:51:52Z

coresimd/src/x86/i586/sse2.rs

@@ -29,7 +28,7 @@ pub unsafe fn _mm_pause() {
 #[inline(always)]
 #[target_feature = "+sse2"]
 #[cfg_attr(test, assert_instr(clflush))]
-pub unsafe fn _mm_clflush(p: *mut c_void) {
+pub unsafe fn _mm_clflush(p: *mut u8) {


Intel specifies that the _mm_clflush intrinsic takes a const void*. I don't know why it takes a *mut c_void, and am slightly uncomfortable with it taking a *mut u8 instead.

The tracking issue to fix this is: rust-lang/rust#36193

I've opened this issue #199 to track the intrinsics in which we break the API because of the dependency on c_void. This should block the stabilization of these intrinsics.

alexcrichton · 2017-11-20T20:32:13Z

Looks like some appveyor tests may be failing? (I'd recommend removing --nocapture from CI to get some better output)

gnzlbg · 2017-11-20T20:35:59Z

@alexcrichton done (btw why does --nocapture produce worse output on windows?)

gnzlbg · 2017-11-20T21:05:12Z

So I've removed --nocapture but the output looks the same.

alexcrichton · 2017-11-20T22:20:40Z

Oh --nocapture isn't windows specific, it, when passed, just means that the otuput of each test is garbled with all the other outputs, but now that it's not passed the output for each test is in its own delineated section.

gnzlbg · 2017-11-20T22:22:56Z

I think that we are testing with only one thread so that shouldn’t matter that much, but anyways I have no clue why the windows test are failing and no window machine to test :/

…

On Mon 20. Nov 2017 at 23:20, Alex Crichton ***@***.***> wrote: Oh --nocapture isn't windows specific, it, when passed, just means that the otuput of each test is garbled with all the other outputs, but now that it's not passed the output for each test is in its own delineated section. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#197 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NpkqBKDgMJXXM7Ek_9mVk5kGAvjc8ks5s4fs_gaJpZM4Qknc5> .

alexcrichton · 2017-11-20T22:25:06Z

So FWIW I just added a [workspace] directive to the root Cargo.toml and it worked? I wonder if the link error you got was maybe due to a stale target directory? Or maybe I'm running tests differently?

I'll test on Windows

gnzlbg · 2017-11-20T22:26:49Z

The linking errors happened on 32 bit targets on Travis :/ Maybe I just did it wrong.

…

On Mon 20. Nov 2017 at 23:25, Alex Crichton ***@***.***> wrote: So FWIW I just added a [workspace] directive to the root Cargo.toml and it worked? I wonder if the link error you got was maybe due to a stale target directory? Or maybe I'm running tests differently? I'll test on Windows — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#197 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3Nph-b01BToRWfwEBDVyKcBYt1iLW2ks5s4fxPgaJpZM4Qknc5> .

alexcrichton · 2017-11-20T22:31:52Z

Aha, I've reproduced the 32-bit failure as well

alexcrichton · 2017-11-20T22:36:19Z

Ok I think I've pushed a fix for workspaces, it was I believe just related to disabling tests for the procedural macros

alexcrichton · 2017-11-20T22:54:39Z

That seems to have... magically fixed windows?

So I can't actually run the tests locally on Windows due to a faulting instruction. The test is failing on the xsaves test and instruction. The disassembly in visual studio when it faults leads with:

00007FF7EBEF990F  mov         edx,0FFFFFFFFh  
00007FF7EBEF9914  mov         eax,0FFFFFFFFh  
00007FF7EBEF9919  xsaves      [rsp+80h]

where the faulting instruction is xsaves and RSP is 000000696F3FB580. So I think that's 64-byte aligned? The reported exception is:

Unhandled exception at 0x00007FF7EBEF9919 in coresimd-80b6104084dfdbe5.exe: 0xC0000005: Access violation reading location 0xFFFFFFFFFFFFFFFF. occurred

I'm not really sure what's going on there... I wonder if maybe xsaves detection isn't working? I apparently have an i7-7700k, although I'm not sure if it actually supports xsaves or not. Do you know what's going on?

gnzlbg · 2017-11-21T10:02:49Z

So I think that's 64-byte aligned?

Yes.

I'm not really sure what's going on there... I wonder if maybe xsaves detection isn't working?

I don't think so, we currently only enable xsaves if the CPU supports AVX and XSAVES, and if the OS supports it (OSXSAVE). Also, Appveyor says that xsaves is not enabled, while on your machine it says that it is enabled, so I think run-time detection looks fine.

Do you know what's going on?

This probably means that the xsaves/xsaves64 tests were always broken.

Two things point in this direction:

The --nocapture results from a previous Appveyor build show that the test for xsaves returns false on Appveyor, and true on your machine, so these were not tested before on Windows.
I disabled xsaves tests on Intel SDE because... I am an idiot, so these are probably not tested on Linux either.

It is weird that you get an error saying that you are trying to read from 0xFFFF.... That looks surprisingly similar to the mask that XSAVES uses, but this mask is not a memory address; it is used to specify which registers should be saved, where 0xFFFF... just means "save all registers".

But IMO the simplest explanation is that they were broken already, and that the bug surfaced only on your machine, so can you comment them out and test if the xsavec tests work?

If so, I would move on with this and I'll try to fix the tests this week (I have some machines with xsaves enabled).

gnzlbg · 2017-11-21T18:42:00Z

@alexcrichton can you post a cat /proc/cpuinfo of that cpu? (not sure how easy it is to do so from windows but maybe you happen to have dual boot?)

gnzlbg · 2017-11-21T19:14:15Z

rebased

alexcrichton · 2017-11-21T21:11:56Z

r=me, but looks like it needs a rebase (feel free to merge after rebasing)

* Enable a Cargo workspace for the repo * Disable tests for proc-macro crates * Move back to mounting source directory read-only * Refactor test invocation to only test one crate with `--all`

gnzlbg commented Nov 20, 2017

View reviewed changes

gnzlbg force-pushed the split branch from efa5a08 to b3209de Compare November 20, 2017 18:41

gnzlbg force-pushed the split branch from b59d620 to 65da2dc Compare November 21, 2017 19:13

gnzlbg mentioned this pull request Nov 21, 2017

enable std by default #194

Closed

gnzlbg force-pushed the split branch 2 times, most recently from 965665c to c13fbc4 Compare November 21, 2017 23:03

gnzlbg added 5 commits November 22, 2017 10:49

refactor no_std components into the coresimd crate

3fc4c61

c_void -> *mut u8

1fabed5

fix doc script

31c2291

[ci] flag the documentation build bot

8597062

Only coresimd depends on stdsimd-test.

5a8322c

gnzlbg force-pushed the split branch from 36cb245 to 148af35 Compare November 22, 2017 09:49

Use workspaces and fix tests

a8d45e4

* Enable a Cargo workspace for the repo * Disable tests for proc-macro crates * Move back to mounting source directory read-only * Refactor test invocation to only test one crate with `--all`

gnzlbg force-pushed the split branch from 148af35 to a8d45e4 Compare November 22, 2017 10:15

Fix clippy and rust-fmt.

926874d

gnzlbg force-pushed the split branch from 6505947 to 926874d Compare November 22, 2017 12:41

gnzlbg merged commit f3ee983 into rust-lang:master Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coresimd] extracts the no_std components into the coresimd crate #197

[coresimd] extracts the no_std components into the coresimd crate #197

gnzlbg commented Nov 20, 2017 •

edited

Loading

gnzlbg Nov 20, 2017

alexcrichton Nov 20, 2017

gnzlbg Nov 20, 2017 •

edited

Loading

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017

gnzlbg Nov 20, 2017 •

edited

Loading

gnzlbg Nov 20, 2017 •

edited

Loading

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 20, 2017

gnzlbg commented Nov 20, 2017

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 20, 2017 via email

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 20, 2017 via email

alexcrichton commented Nov 20, 2017

alexcrichton commented Nov 20, 2017

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 21, 2017 •

edited

Loading

gnzlbg commented Nov 21, 2017

gnzlbg commented Nov 21, 2017

alexcrichton commented Nov 21, 2017

[coresimd] extracts the no_std components into the coresimd crate #197

[coresimd] extracts the no_std components into the coresimd crate #197

Conversation

gnzlbg commented Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg Nov 20, 2017 • edited Loading

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 Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

gnzlbg Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 20, 2017

gnzlbg commented Nov 20, 2017

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 20, 2017 via email

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 20, 2017 via email

alexcrichton commented Nov 20, 2017

alexcrichton commented Nov 20, 2017

alexcrichton commented Nov 20, 2017

gnzlbg commented Nov 21, 2017 • edited Loading

gnzlbg commented Nov 21, 2017

gnzlbg commented Nov 21, 2017

alexcrichton commented Nov 21, 2017

gnzlbg commented Nov 20, 2017 •

edited

Loading

gnzlbg Nov 20, 2017 •

edited

Loading

gnzlbg Nov 20, 2017 •

edited

Loading

gnzlbg Nov 20, 2017 •

edited

Loading

gnzlbg commented Nov 21, 2017 •

edited

Loading