Reduce monomophic versions of bswap in DataView #1717
Conversation
MaxGraey
commented
Mar 6, 2021
MaxGraey
commented
- I've read the contributing guidelines
| } else if (sizeof<T>() == 4) { | ||
| return <T>(((value << 8) & <T>0xFF00) | ((value >> 8) & <T>0x00FF) | (value & <T>0xFFFF0000)); | ||
| return <T>(((value << 8) & <T>0xFF00) | ((value >>> 8) & <T>0x00FF) | (value & <T>0xFFFF0000)); |
There was a problem hiding this comment.
Wondering a bit about the usefulness of bswap16, since we are not using it anywhere. What are typical use cases for it?
There was a problem hiding this comment.
I don't know but post-MVP suggest add it: https://github.com/WebAssembly/design/blob/master/FutureFeatures.md#additional-integer-operators
There was a problem hiding this comment.
I see, hmm. If I'm not mistaken, semantics there seem a little different tho in that high bits are discarded.
There was a problem hiding this comment.
yes. bswap16 preserve high part of value while bswap<u16> don't
There was a problem hiding this comment.
Do you think it would be possible to integrate it into bswap, as bswap<u16>, given the assumption that discarding high bits is fine? For i16, we'd then also sign extend after swapping I guess. (Doesn't need to be done in this PR)
There was a problem hiding this comment.
I think we could remove swap16 at least for now. I don't remind where it needed. Anyway this easily implement in user space if it really required in some project. wdyt?
There was a problem hiding this comment.
Yeah, I guess we could do this similar to other generic builtins. For instance, load and load16_s also become just a generic load<T>, and I'd expect bswap and bswap16 to become a single bswap<T> as well. On bswap<i16>, the input is already known to have garbage in the high bits, so doing either just a bswap16 on unsigned or a bswap16 + sign-extend on signed seems sufficient.
