Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start testing runtime execution of wasm32 simd intrinsics (take 2) #740

Merged
merged 4 commits into from
Apr 25, 2019

Conversation

alexcrichton
Copy link
Member

This commit starts to add support to CI to not only compile all wasm32 SIMD intrinsics but also actually execute some of them at runtime. Node 12 has an update of v8 which aligns internal opcode definitions with the most recent version of the wasm SIMD spec. As a result we can start actually executing some WebAssembly code with SIMD instructions in it!

Unfortunately not all SIMD instructions are implemented in v8 right now. In LLVM this is known as the +simd128 feature (what v8 implements) and the +unsupported-simd128 (everything v8 doesn't implement but is spec'd). We continue to compile the library with both features (to make sure it works) but this PR then also adds a mdoe where it compiles with only +simd128 and executes the resulting binary in node.js. Unfortunately compiling with just +simd128 results in lots of LLVM errors, so we also compile with --cfg only_node_compatible to statically remove non-node-compatible functions.

The end result is that we're successfully executing all #[assert_instr] annotations! Additionally I've started uncommenting some tests in mod tests and updating them to the new intrinsic names. There's still a lot more work to be done there, but I figured it'd be good to post this for initial feedback.

In general, the changes here are:

  • Compile a version of Node.js with wasm support (can download binaries once node 12 is released)
  • Update CI to dynamically download the right version of wasm-bindgen instead of pinning
  • Add #[cfg] to optionally remove SIMD which isn't implemented in node.js
  • Add CI configuration to execute select SIMD tests in node.js

This is a follow-up to #715

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 24, 2019

FYI emscripten already has a header (and tests) for some of these :)

@alexcrichton
Copy link
Member Author

Perhaps yeah, but I'm not sure what relation that has to this PR?

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 24, 2019

I thought it might be interesting to inspect the tests there to see if anything that's still commented out here already should work.

@alexcrichton
Copy link
Member Author

I started getting some of them passing in this PR yeah but it's going to take a lot of work to uncomment them since the interface has changed since they were first written and it's a lot of macros to update. I was going to try tackling that when I had time later but wanted to get this landed first.

@tlively
Copy link

tlively commented Apr 24, 2019

I am very interested in the LLVM failures you’re seeing when compiling with +simd128. Can you share more details?

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 25, 2019

@tlively everything that's commented out here did not work last time we tried, this PR enables some of those tests, but we really should be filling bugs for what's not working yet.

We have been low on filling LLVM bugs for those cases, but if you want to explore these it might be better for you to either play with those in godbolt (I can help you set up a godbolt environment that allows you to easily play with them), or to try to compile these with the rust toolchain (I can also help you here, you only need nightly rust, and compiling the core_arch crate here for the wasm32-unknown-unknown target). I'm on IRC/Discord/Zulip if you want to chat about it. IIRC there were also some issues with generic intrinsics (e.g. gather scatters) that had LLVM bugs filled for wasm32 but were hard to reproduce.

@alexcrichton
Copy link
Member Author

alexcrichton commented Apr 25, 2019

@tlively sure yeah! To be clear though I didn't think these were bugs in LLVM because while LLVM may have errors with +simd128 it doesn't with +simd128,+unimplemented-simd128 as well so I figured all the support is there and there's not too much interest in getting everything to work on just +simd128 because these features should be merged at some point.

That being said there's a bunch of functions here tagged as #[cfg(not(only_node_compatible_functions))]. Those either fail in +simd128 (note that this mode isn't using +unimplemented-simd128, we have another mode for that elsewhere) because LLVM generates a polyfill which doesn't contain the instruction we want (aka i64x2_splat generates a bunch of instructions which aren't i64x2.splat but are semantically the same) or because LLVM's code generator crashes.

The easiest way to see them is to work with this PR directly, but in the spirit of a shortcut an example is the i64x2.neg instruction:

define <4 x i32> @i64x2_neg(<4 x i32> %a) {
  %a2 = bitcast <4 x i32> %a to <2 x i64>
  %neg1 = insertelement <2 x i64> undef, i64 -1, i32 0
  %neg2 = shufflevector <2 x i64> %neg1, <2 x i64> undef, <2 x i32> zeroinitializer
  %negated = mul <2 x i64> %a2, %neg2
  %r = bitcast <2 x i64> %negated to <4 x i32>
  ret <4 x i32> %r
}

That, when compiled with +simd128 fails to generate code with an "LLVM ERROR". (note that we're also using the ~8.0 LLVM release, so if later work has happened on the SIMD backend we're missing that). Now when the code is compiled with +simd128,+unimplemented-simd128 it compiles as expected! (and generates i64x2.neg).

Because all the functions work with +unimplemented-simd128 (and I've checked manually they generate the expected instructions) I'm not under the impression there's much to do from LLVM's point of view. Eventually a wasm runtime will implement all the SIMD instructions and we'll only have +simd128 instead of two features and it should all check out!

@alexcrichton
Copy link
Member Author

@gnzlbg FWIW I think this is ready to go, I think CI failures were introduced in the recent rdtsc change?

@gnzlbg gnzlbg merged commit e9b425f into rust-lang:master Apr 25, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 25, 2019

Yeah, I have a fix for that, but it's in #737 .

@alexcrichton alexcrichton deleted the wasm-stuff branch May 23, 2019 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants