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.
-->
jlb6740 requested cfallin, abrown, bnjbvr and julian-seward1 for a review on PR #2302.
cfallin submitted PR Review.
cfallin submitted PR Review.
cfallin created PR Review Comment:
s/converted/converting/
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.
jlb6740 submitted PR Review.
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:
jlb6740 submitted PR Review.
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.
cfallin submitted PR Review.
cfallin created PR Review Comment:
Ah, OK, no worries and thanks!
jlb6740 closed without merge PR #2302.
Last updated: Dec 23 2024 at 12:05 UTC