Stream: git-wasmtime

Topic: wasmtime / PR #2234 Adds packed floating point min max

Wasmtime GitHub notifications bot (Sep 25 2020 at 23:58):

jlb6740 opened PR #2234 from add-packed-float-min-max 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 (Sep 28 2020 at 03:52):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 28 2020 at 06:10):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 28 2020 at 06:14):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 28 2020 at 06:59):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 28 2020 at 07:00):

jlb6740 has marked PR #2234 as ready for review.

Wasmtime GitHub notifications bot (Sep 28 2020 at 07:03):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 28 2020 at 07:09):

jlb6740 requested bnjbvr for a review on PR #2234.

Wasmtime GitHub notifications bot (Sep 28 2020 at 07:09):

jlb6740 requested cfallin and bnjbvr for a review on PR #2234.

Wasmtime GitHub notifications bot (Sep 28 2020 at 11:45):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:37):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:37):

abrown created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:38):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:38):

abrown created PR Review Comment:

Remove?

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:40):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:40):

abrown created PR Review Comment:

Bill Budge showed me this: https://chromium-review.googlesource.com/c/v8/v8/+/2231653/1.

Wasmtime GitHub notifications bot (Sep 30 2020 at 06:23):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 30 2020 at 06:26):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 30 2020 at 06:33):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 30 2020 at 06:33):

jlb6740 created PR Review Comment:

@abrown Excellent, thanks! Even without workloads to truly confirm I think this is a definite future todo.

Wasmtime GitHub notifications bot (Sep 30 2020 at 06:43):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 (Sep 30 2020 at 06:43):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 30 2020 at 06:43):

jlb6740 created PR Review Comment:

:+1: Yep, thanks. Good catch.

Wasmtime GitHub notifications bot (Sep 30 2020 at 16:57):

jlb6740 requested cfallin and bnjbvr for a review on PR #2234.

Wasmtime GitHub notifications bot (Sep 30 2020 at 16:57):

jlb6740 requested cfallin, abrown and bnjbvr for a review on PR #2234.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

I've been using the following (not that it makes much of a difference logically but for clarity):

            if !output_ty.is_vector() {

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    // X64 handles propagation of -0's and Nans differently between left and right

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                // scalar approach we use jumps to handle cases where NaN and +0 propagation is

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    let tmp_xmm1 = ctx.alloc_tmp(RegClass::V128, output_ty);

I don't think this will affect the codegen but it seems more correct: we want this register to contain a vector type no a scalar type.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

:+1: for bringing the other tests in as well; we can uncomment the others as we get to them. I do think we should add compile tests for fmin/fmax like I did in simd-lane-access-compile.clif; that way we test that the sequence emitted stays correct over time. I don't think we should do it for simpler stuff but for long, tricky sequences like these I feel it would be helpful.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    ctx.emit(Inst::xmm_rm_r(or_op, RegMem::from(dst), tmp_xmm1));

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

And the same a few other places below.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    // operands. After doing the min in both directions, this OR will

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                // First we perform the Min/Max in both directions. This is because in the

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    // X64 handles propagation of -0's and Nans differently between left and right
                    // operands. After doing the max in both directions, this OR will
                    // guarantee capture of 0's and Nan in our tmp register.
                    ctx.emit(Inst::xmm_rm_r(or_op, RegMem::reg(dst.to_reg()), tmp_xmm1));

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    ctx.emit(Inst::xmm_rm_r(min_op, RegMem::from(dst), tmp_xmm1));

I added RegMem::from to make things less verbose.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

                    // guarantee capture of -0's and Nan in our tmp register

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

I think we should copy in some of the documentation I had in expand_minmax_vector to explain why we are shifting this certain number of bits. Without that, it may be unclear to someone looking at this later, especially the part where we always produce -NaN here and that is allowed by the spec--that is not very obvious.

Wasmtime GitHub notifications bot (Oct 01 2020 at 18:21):

abrown created PR Review Comment:

nit: I know @bnjbvr would want these to have ending periods.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:53):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:53):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:54):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:54):

jlb6740 created PR Review Comment:

:+1: .. I was just using jmps for shorthand. Saves a lot of characters :-).

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:54):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:54):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:54):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:54):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 17:55):

jlb6740 created PR Review Comment:

:+1: Thought I'd removed that comment.

Wasmtime GitHub notifications bot (Oct 02 2020 at 21:07):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 02 2020 at 22:19):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 22:19):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 22:19):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 22:19):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 22:19):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 22:19):

jlb6740 created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Oct 02 2020 at 22:21):

jlb6740 updated PR #2234 from add-packed-float-min-max 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 02 2020 at 23:18):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 02 2020 at 23:18):

jlb6740 created PR Review Comment:

:+1:

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

jlb6740 merged PR #2234.

Last updated: Dec 23 2024 at 12:05 UTC