Convert packed floating point to signed integer by jlb6740 · Pull Request #2320 · bytecodealliance/wasmtime

jlb6740 · 2020-10-25T23:25:35Z

No description provided.

abrown · 2020-10-26T16:56:40Z

+
+                // Get the low 16 bits
+                ctx.emit(Inst::xmm_rmi_reg(SseOpcode::Pslld, RegMemImm::imm(16), tmp));
+                ctx.emit(Inst::xmm_rmi_reg(SseOpcode::Psrld, RegMemImm::imm(16), tmp));


I used PBLENDW in the old backend and so does V8... what do you think?

Ahh .. yes. V8 uses pblendw as well. This adds an extra instruction; both pblendw and pslld/psrld have the same instruction latency. I choose not to use pblendw though because it is only compatible with SSE4_1 or greater while shifts are compatible with SSE2 which I thought was the base target for SIMD. In general not sure how in the new backend we are guarding lowering based on compatibility level so for now I am lowering based on the lowest denominator. What do you think??

Ah, cool--having both would actually be great: if ...has_sse41() { [emit pblendw] } else { [emit the double shift] }. That information is available in EmitInfo.isa_flags but unfortunately that struct is not present until the emit phase. If we created a new Inst::XmmLowBits { dst, bits_to_retain } (or something like that) and lowered to that then in the emit phase we could pick which version we want based on the EmitInfo. The other option would be to try to make those ISA flags available during lowering but that seems harder to do (@cfallin?).

abrown · 2020-10-26T17:00:45Z

+                        tmp,
+                    ));
+
+                    // Convert the float to double.


Suggested change

// Convert the float to double.

// Convert the float to double quadword.

By double I meant double word. This is converting to a double word though not a double quadword. I'll change it to say packed doubleword.

I think the logic looks right but can we add the CLIF tests that verify these individual instructions? I'm thinking that simd-conversion-run.clif and simd-conversion-legalize.clif (once converted to being a test compile emitting vcode) would be very useful to see that each instruction works correctly and compiles to the sequence we expect.

It definitely passes the SIMD Spectest so I am confident it is correct but let me look to add a file test as well. 👍

These instructions are tested in simd_conversions.wast but this file has not been enabled in experimental_x64_should_panic in the build.rs so I don't think any spec tests are running for these instructions. Unfortunately, that spec test also checks narrow and widen which I found a bit annoying; a lot has to be implemented for the spec test to be enabled. So I guess the CLIF file tests will be the only things checking this until all conversions are implemented.

Yes, you're right they aren't enabled by default. I ran them manually though, basically removed all tests except the ones related to the packed float conversion to packed signed int. I also confirmed that it was indeed running as expected while testing. Separately I've also included the file tests that have tests for this conversion .. commenting out tests packed float to packed unsigned int which isn't supported yet.

abrown · 2020-10-26T17:04:00Z

+                    unimplemented!("f32x4.convert_i32x4_u");
+                } else {
+                    unreachable!();
+                }


unreachable! is not correct here because there are two other opcodes that could reach this: Opcode::FcvtToUint | Opcode::FcvtToSint; perhaps we should do a match on all four opcodes (filling the others with unimplemented!()) and then _ => unreachable!() at the end.

I think I disagree though I may be wrong. This code is guarded by a check for vector instructions and so afaik neither Opcode::FcvtToUint or Opcode::FcvtToSint are supported for vector input. In the context of a vector instruction it currently impossible to reach this branch with those opcodes right? This question has come up before where I reach for using unreachable instead of implementing it as unimplemented. I can change to unimplemented but not really sure the rules for applying unimplemented vs unreachable when context is considered. Certainly support for vector input for Opcode::FcvtToUint or Opcode::FcvtToSint could be added, but then that is the case for most places in the backend were the unreachable! is used instead of unimplemented! For example there are places where we match on a type (pshufd use in extractlane for example) and say the default _ => is unreachable simply because a type is not supported, but if there was need for that support and that support were added it is suddenly unimplemented and not unreachable.

If Opcode::FcvtToUint and Opcode::FcvtToSint are not supported then this should remain unreachable!; maybe add a note because a straightforward reading of the code would expect these to be implemented.

abrown

I think the logic looks right but can we add the CLIF tests that verify these individual instructions? I'm thinking that simd-conversion-run.clif and simd-conversion-legalize.clif (once converted to being a test compile emitting vcode) would be very useful to see that each instruction works correctly and compiles to the sequence we expect.

jlb6740 · 2020-10-27T01:00:34Z

Hopefully all issues have been addressed, but let me know if there is anything else.

abrown

LGTM, see comments. @cfallin, any thoughts on a way to determine what SSE features are available during lowering?

abrown · 2020-10-28T16:56:58Z

@@ -0,0 +1,34 @@
+test legalizer
+set enable_simd
+target x86_64 skylake


This is currently running the old backend; I think it should be modified to test compile and add feature "experimental_x64" (see simd-bitwise-compile.clif, e.g.).

Ok .. Yeah thanks. Will make this change too. It somehow was being acknowledged as testing was failing CI when I had the file tests for conversion to unsigned included, but I was having trouble running it on my machine. Will update.

@abrown @bnjbvr .. Actually I am going to just remove this compile file test. It is checking for a very specific sequence of instructions which should not be static (set in stone). It will depend on optimizations or SSE feature flag set and is there anything else that can change register allocation even if the same instructions are used?

abrown · 2020-10-28T16:58:12Z

+                    // Since this branch is also guarded by a check for vector types
+                    // neither Opcode::FcvtToUint nor Opcode::FcvtToSint can reach here
+                    // as the first to branches will cover all reachable cases.


Suggested change

// Since this branch is also guarded by a check for vector types

// neither Opcode::FcvtToUint nor Opcode::FcvtToSint can reach here

// as the first to branches will cover all reachable cases.

// Since this branch is also guarded by a check for vector types,

// neither Opcode::FcvtToUint nor Opcode::FcvtToSint can reach here

// (the vector variants do not exist).

Implements i32x4.trunc_sat_f32x4_s

Add portions of filetests simd-conversion-legalize.clif and simd-conversion-run.clif that test fcvt_from_sint.f32x4

bnjbvr · 2020-10-28T18:58:21Z

LGTM, see comments. @cfallin, any thoughts on a way to determine what SSE features are available during lowering?

X64Backend has a x64_flags field which contains this information, e.g. has_sse42() (as derived from the meta, x64 target-specific settings file); you could access it by changing slightly the signature of lower_insn_to_regs and passing it the self.x64_flags next to the self.flags.

Adds support for converting packed unsigned integer to packed float

1e4de57

github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Oct 25, 2020

jlb6740 marked this pull request as ready for review October 26, 2020 03:44

jlb6740 requested review from abrown, cfallin and julian-seward1 and removed request for cfallin October 26, 2020 03:44

jlb6740 force-pushed the convert_sat branch from 9510d8a to 3d1cbb9 Compare October 26, 2020 04:39

abrown reviewed Oct 26, 2020

View reviewed changes

Comment thread cranelift/codegen/src/isa/x64/lower.rs

abrown reviewed Oct 26, 2020

View reviewed changes

Comment thread cranelift/codegen/src/isa/x64/lower.rs

abrown reviewed Oct 26, 2020

View reviewed changes

abrown suggested changes Oct 26, 2020

View reviewed changes

jlb6740 force-pushed the convert_sat branch 2 times, most recently from b5e9a14 to 9b43733 Compare October 27, 2020 00:59

jlb6740 requested a review from abrown October 27, 2020 00:59

jlb6740 force-pushed the convert_sat branch from 9b43733 to 1bf81da Compare October 27, 2020 05:28

abrown approved these changes Oct 28, 2020

View reviewed changes

jlb6740 added 2 commits October 28, 2020 11:18

Add support for packed float to signed int conversion

b6d19d5

Implements i32x4.trunc_sat_f32x4_s

Add filetests for fcvt_from_sint.f32x4

5c764c4

Add portions of filetests simd-conversion-legalize.clif and simd-conversion-run.clif that test fcvt_from_sint.f32x4

jlb6740 force-pushed the convert_sat branch from 1bf81da to 5c764c4 Compare October 28, 2020 19:27

jlb6740 merged commit fa66dae into bytecodealliance:main Oct 28, 2020

jlb6740 mentioned this pull request Oct 28, 2020

Revert "Convert packed floating point to signed integer " #2333

Closed

	// Convert the float to double.
	// Convert the float to double quadword.

Conversation

jlb6740 commented Oct 25, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlb6740 Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlb6740 Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrown left a comment

Choose a reason for hiding this comment

Uh oh!

jlb6740 commented Oct 27, 2020

Uh oh!

abrown left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnjbvr commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jlb6740 Oct 26, 2020 •

edited

Loading

jlb6740 Oct 27, 2020 •

edited

Loading