Stream: git-wasmtime

Topic: wasmtime / PR #2302 Packed unsigned int to float conversion

Wasmtime GitHub notifications bot (Oct 19 2020 at 06:42):

jlb6740 edited PR #2302 from packed-convert-unsigned-int2 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 21 2020 at 17:56):

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

Wasmtime GitHub notifications bot (Oct 28 2020 at 04:42):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 04:42):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 28 2020 at 04:42):

cfallin created PR Review Comment:

s/converted/converting/

Wasmtime GitHub notifications bot (Oct 28 2020 at 04:42):

cfallin created PR Review Comment:

This is quite clever! A few questions:

• This all assumes 32x4, right? Do we also need to support 64x2 (converting into doubles rather than floats)? If not, can we add an assert (not just a debug assert; we should check in release too)?

• I was quite confused at first as I had thought (from this comment) that this was somehow using a 16-bit conversion rather than a 32-bit conversion with either the upper or lower 16 bits zeroed. I think it would be helpful to add some pseudocode or even ASCII art or something to illustrate what's going on -- for each lane, we basically have:

upper := (src & 0xffff0000) >> 1
lower := (src & 0x0000ffff)

dst := float(upper)*2 + float(lower)

which makes it a little easier to see the equivalence.

Wasmtime GitHub notifications bot (Oct 29 2020 at 23:45):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 29 2020 at 23:45):

jlb6740 created PR Review Comment:

This all assumes 32x4, right? Do we also need to support 64x2 (converting into doubles rather than floats)? If not, can we add an assert (not just a debug assert; we should check in release too)?

Yes, unsigned into float is only 32x4 for WASM. I can add a guard. Also I can add some additional comment/illustration to explain the algorithm better. :+1:

Wasmtime GitHub notifications bot (Oct 29 2020 at 23:47):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (Oct 29 2020 at 23:47):

jlb6740 created PR Review Comment:

@cfallin Also, this patch submitted as part of different patch set (branch) without realizing the impact. So patch merged yesterday so I'll address the comments in a separate patch from this one.

Wasmtime GitHub notifications bot (Oct 30 2020 at 00:21):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 30 2020 at 00:21):

cfallin created PR Review Comment:

Ah, OK, no worries and thanks!

Wasmtime GitHub notifications bot (Oct 30 2020 at 19:07):

jlb6740 closed without merge PR #2302.

Last updated: Nov 22 2024 at 16:03 UTC