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

Uncomment wasm32 bitmask instruction #1004

Closed
wants to merge 12 commits into from

Conversation

Daniel-Liu-c0deb0t
Copy link
Contributor

#874 introduced some iNNxMM_bitmask instructions that were supposed to be supported in LLVM 11, but they were commented out. Since Rust is already on LLVM 11 (https://blog.rust-lang.org/2020/10/08/Rust-1.47.html), this PR uncomments those instructions so they can be used.

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@Amanieu
Copy link
Member

Amanieu commented Feb 14, 2021

cc @alexcrichton: This looks good to me but I want to run it by you first since it's WASM-related.

@Daniel-Liu-c0deb0t
Copy link
Contributor Author

Saw some other commented out lines for the abs instructions and i64x2_mul, so I uncommented those as well.

@Daniel-Liu-c0deb0t Daniel-Liu-c0deb0t marked this pull request as ready for review February 14, 2021 03:56
@Daniel-Liu-c0deb0t
Copy link
Contributor Author

The tests for WASM don't seem to be running. Is there a problem with them?

@Amanieu
Copy link
Member

Amanieu commented Feb 14, 2021

I believe it was commented it in the CI configuration because it was failing and never got re-enabled. You can try re-enabling it.

@Daniel-Liu-c0deb0t
Copy link
Contributor Author

Daniel-Liu-c0deb0t commented Feb 14, 2021

I'll have to play around with enabling and updating the CI config and I'll likely open another PR for that.

Edit: PR at #1006
If wasmtime is not able to handle the new WASM SIMD instructions, then we should try wasmer.

@Amanieu
Copy link
Member

Amanieu commented Feb 14, 2021

Can you rebase to see if it passes WASM CI?

@Amanieu Amanieu assigned Amanieu and unassigned Amanieu Feb 14, 2021
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

FWIW wasm simd and LLVM are still sort of constantly moving targets. The simd proposal for wasm is nearing the next phase of stabilization, however, so I hope that it will settle down in the near future. Once everything settles down again on the wasm side I plan to make another pass through this module so sync with the current state of the spec (which has renamed, added, and tweaked various instructions)

For now though looks great!

@Daniel-Liu-c0deb0t
Copy link
Contributor Author

Daniel-Liu-c0deb0t commented Feb 15, 2021

The reason why I want bitmask right now is because I want to use it in a project I'm working on.

Running the CI (wasmtime) results in weird cranelift errors that look like:

thread '<unnamed>' panicked at 'ABI argument v4 should be in a register', cranelift/codegen/src/regalloc/coloring.rs:409:21

Not sure what to do, as I'm not familiar with this.

Edit: tried wasmer, but it isn't up to date as the bitmask operations aren't supported. Latest cranelift version seems to support them, though (https://github.com/bytecodealliance/wasmtime/blob/09b976e1d53a05150c7b8acd36a82b34cec787b3/cranelift/wasm/src/code_translator.rs#L1639), so I'm back to trying wasmtime. Regardless, i64x2_bitmask doesn't seem to be supported, so I commented it out.

@Daniel-Liu-c0deb0t
Copy link
Contributor Author

Daniel-Liu-c0deb0t commented Feb 15, 2021

Running tests with the latest wasmtime dev release results in these errors:

thread '<unnamed>' panicked at 'ABI argument v5 should be in a register', cranelift/codegen/src/regalloc/coloring.rs:409:21
thread '<unnamed>' panicked at 'not implemented: Currently only SIMD instructions are mapped to their return type; the following instruction is not mapped: V128AnyTrue', cranelift/wasm/src/code_translator.rs:2688:14

@alexcrichton any idea what is going on?

@alexcrichton
Copy link
Member

The SIMD support in Wasmtime is still experimental and not fully fleshed out, so you're probably hitting a bug of a case that isn't implemented yet or just a bug in the implementation. If possible could you minimize the case and report it to rust-lang/rust?

@Daniel-Liu-c0deb0t
Copy link
Contributor Author

Daniel-Liu-c0deb0t commented Mar 1, 2021

Another potential thing that should be fixed: missing target feature attributes for some WASM SIMD functions, which affects inlining. Though this should be less of a problem once rust-lang/rust#74320 is fixed.

@bors
Copy link
Contributor

bors commented Mar 11, 2021

☔ The latest upstream changes (presumably 8724eb4) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

I believe that wasm simd stuff is all up to date by this point, so I think this can be closed now?

@Amanieu Amanieu closed this May 19, 2021
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.

5 participants