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

Implement fcvt_to_uint_sat (f32x4 -> i32x4) for x86 #1990

Merged
merged 3 commits into from
Jul 8, 2020

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Jul 7, 2020

This replaces #1822; it consists of the same functionality but removes the AVX512 instruction lowering for the time being. There are two reasons for this:

  • the default MXCSR rounding is round to nearest even, which does not match the semantics required by i32x4.trunc_sat_f32x4_u. We can then use embedded rounding control but lose the ability to specify the vector length, so the instruction would operate on 512-bits which we should discuss (@sunfishcode has reported issues with 512-bit vectors in Spidermonkey)
  • the output of VCVTPS2UDQ for negative lanes is 0xFFFFFFFF (I had thought it would be 0x00000000); this can be resolved with the following sequence: v0 = pxor ...; v2 = fcmp gte v1, v0 (gte ensures they are ordered); v3 = vcvtps2udq v1; v4 = band v2, v3. However, I would like to look at this a little bit more before submitting a separate PR for it (this is the reason for keeping the legalization in enc_tables.rs and under narrow_avx, BTW).

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. cranelift:wasm labels Jul 7, 2020
@github-actions
Copy link

github-actions bot commented Jul 7, 2020

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:wasm"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Contributor

@julian-seward1 julian-seward1 left a comment

Choose a reason for hiding this comment

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

Ok to land, but please remove the redundant mention of AVX512 in the commit message:
"This converts an f32x4 into an i32x4 (unsigned) with some rounding either by using an AVX512VL/F instruction--VCVTPS2UDQ--or a long sequence of SSE4.1 compatible instructions."

Thanks for your patience with this!

This converts an `f32x4` into an `i32x4` (unsigned) with rounding by using a long sequence of SSE4.1 compatible instructions.
@abrown abrown merged commit 5c35a96 into bytecodealliance:main Jul 8, 2020
@abrown abrown deleted the trunc-sat-unsigned-again branch July 8, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:meta Everything related to the meta-language. cranelift:wasm cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants