Stream: git-wasmtime

Topic: wasmtime / PR #2320 Convert packed floating point to sign...

Wasmtime GitHub notifications bot (Oct 25 2020 at 23:25):

jlb6740 opened PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 26 2020 at 03:44):

jlb6740 has marked PR #2320 as ready for review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 03:44):

jlb6740 requested cfallin for a review on PR #2320.

Wasmtime GitHub notifications bot (Oct 26 2020 at 03:44):

jlb6740 requested abrown for a review on PR #2320.

Wasmtime GitHub notifications bot (Oct 26 2020 at 03:44):

jlb6740 requested cfallin and abrown for a review on PR #2320.

Wasmtime GitHub notifications bot (Oct 26 2020 at 04:08):

jlb6740 requested cfallin, abrown and julian-seward1 for a review on PR #2320.

Wasmtime GitHub notifications bot (Oct 26 2020 at 04:39):

jlb6740 updated PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:23):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:23):

abrown created PR Review Comment:

I know this isn't code you wrote, but in the old backend we were using CVTTSI2S{S|D} I believe. It would seem like, for both vector and scalar, we should be using the same class of instruction for both: either CVT... _or_ CVTT...?

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:31):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:31):

abrown created PR Review Comment:

For reference, here is the old backend expand_fcvt_from_uint function with the scalar sequence. I would guess most of that is covered in Inst::cvt_u64_to_float_seq below.

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:38):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:38):

abrown created PR Review Comment:

Oh, I see what I missed: CVTT... is being used for FcvtToSintSat, not FcvtFromUint--disregard :smile:.

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:56):

abrown created PR Review Comment:

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

Wasmtime GitHub notifications bot (Oct 26 2020 at 16:56):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:00):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:00):

abrown created PR Review Comment:

For consistency, Andps?

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:00):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:00):

abrown created PR Review Comment:

                    // Convert the float to double quadword.

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:04):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:04):

abrown created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Oct 26 2020 at 17:06):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:20):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:20):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:29):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:29):

jlb6740 created PR Review Comment:

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??

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:34):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:34):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:37):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:37):

jlb6740 created PR Review Comment:

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 double word.

Wasmtime GitHub notifications bot (Oct 26 2020 at 19:38):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:00):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:00):

jlb6740 created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:02):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:02):

jlb6740 created PR Review Comment:

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. :+1:

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:54):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:54):

abrown created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:59):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 20:59):

abrown created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Oct 26 2020 at 21:06):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 26 2020 at 21:06):

abrown created PR Review Comment:

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?).

Wasmtime GitHub notifications bot (Oct 26 2020 at 23:15):

jlb6740 updated PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:49):

jlb6740 updated PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:52):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:52):

jlb6740 created PR Review Comment:

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. 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.

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:53):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:53):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:59):

jlb6740 updated PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 27 2020 at 00:59):

jlb6740 requested cfallin, abrown and julian-seward1 for a review on PR #2320.

Wasmtime GitHub notifications bot (Oct 27 2020 at 05:28):

jlb6740 updated PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 28 2020 at 16:59):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 16:59):

abrown created PR Review Comment:

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.).

Wasmtime GitHub notifications bot (Oct 28 2020 at 16:59):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 16:59):

abrown created PR Review Comment:

                    // 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).

Wasmtime GitHub notifications bot (Oct 28 2020 at 18:48):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 18:48):

jlb6740 created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Oct 28 2020 at 18:49):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 18:49):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 28 2020 at 19:23):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 19:23):

jlb6740 created PR Review Comment:

@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?

Wasmtime GitHub notifications bot (Oct 28 2020 at 19:27):

jlb6740 updated PR #2320 from convert_sat to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 28 2020 at 20:02):

jlb6740 merged PR #2320.

Last updated: Nov 22 2024 at 17:03 UTC