Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Introduce unsigned narrowing with signed inputs #95

Closed
wants to merge 2 commits into from

Conversation

penzn
Copy link
Contributor

@penzn penzn commented Aug 15, 2019

Narrowing signed inputs to unsigned outputs is widely used in image filtering, when output of a transformation with signed integer coefficients is packed into an array of unsigned integers. The operation is supported in hardware on both x86(64) and ARM.

This PR renames i8x16.narrow_i16x8_u to i8x16.narrow_u16x8_u and i16x8.narrow_i32x4_u to i16x8.narrow_u32x4_u, highlighting the unsigned-to-unsigned semantics; it adds two new opcodes for i8x16.narrow_i16x8_u and i16x8.narrow_i32x4_u for unsigned narrowing with signed inputs.

Instructions corresponding to narrowing of signed values to unsigned results are VQMOVUN on ARM and PACKUSDW on x86; signed to signed narrowing is VQMOVN on ARM and PACKSSDW on x86; unsigned to unsigned is also VQMOVN on ARM (using a different data type) without exact match in x86 SIMD.

For issue #94

@dtig
Copy link
Member

dtig commented Aug 15, 2019

Thanks for the PR, could you add a comment here with mappings to the exact instructions on different architectures? This information is split right now across multiple threads, so having this here for folks to review may be useful.

@penzn
Copy link
Contributor Author

penzn commented Aug 15, 2019

Instructions corresponding to narrowing of signed values to unsigned results are VQMOVUN on ARM and PACKUSDW on x86.

For more context on the instructions already added, not in scope of this change, signed to signed narrowing is VQMOVN on ARM and PACKSSDW on x86; unsigned to unsigned is also VQMOVN on ARM (using a different data type) without exact match in x86 SIMD.

@penzn
Copy link
Contributor Author

penzn commented Aug 20, 2019

Ping @dtig @arunetm

@dtig
Copy link
Member

dtig commented Aug 21, 2019

Given that this has been contentious and the discussion in PR #91, I suggest we put this to vote and resolve this after a reasonable time to vote, possibly in next week's sync meeting. I think the options here are -

If you have opinions about the semantics of narrowing operations, please vote!

@AndrewScheidecker
Copy link
Contributor

FYI I don't see a way to react to your post with 👌. I propose using 👀 instead.

I think there's a clear case for the signed->unsigned narrowing instructions, since there's lots of code out there that uses the corresponding SSE instruction, and NEON has a direct equivalent.

The case for unsigned->unsigned narrowing is less clear. I can imagine it being useful, and hard to reproduce the optimal SSE code-gen for it without a dedicated instruction. It would be nice to have a kernel that benefits from the unsigned->unsigned narrowing instructions to prove that.

I'm leaning toward 👌 (👀) but I'd also be fine with 🚀.

@dtig
Copy link
Member

dtig commented Aug 22, 2019

Oops, thanks for pointing that out. Updated to use 👀 instead of 👌.

@penzn
Copy link
Contributor Author

penzn commented Aug 22, 2019

I prefer the instruction names from this PR, as they make inputs and outputs more explicit, aside of that I am fine with #91 as well.

Also leaning towards this one 👀, but would not oppose removing purely unsigned instructions (with renaming) 🚀

@arunetm arunetm mentioned this pull request Aug 26, 2019
@dtig
Copy link
Member

dtig commented Aug 28, 2019

I've been out this past week so haven't had a chance to follow up with the folks from the 2017 meeting where these were discussed. As there has been general consensus that we should include the signed operations, I'd be in favor of merging #91, and I'll take an AI to dig up the slides and follow up about unsigned operations.

@penzn, The naming could be better, but if we don't end up merging unsigned conversions renaming is possibly not needed?

Approved #91 for merge as the signed narrowing operations have most consensus, and there are concerns about use cases for the unsigned versions of these operations.

Use opcodes from unsigned narrowing for `unsigned narrowing with unsigned
inputs`, as that was the semantics assumed by the spec and inmplemented
in the runtimes. Unsigned narrowing with signed inputs is intended to be
the new instruction introduced in the previous change.
@penzn
Copy link
Contributor Author

penzn commented Aug 28, 2019

@dtig looks like I had some issues with opcodes, pushed an extra commit and updated the top comment. Yes, if we are merging #91, renaming should not be necessary (as there would be no unsigned inputs), just the semantics would change.

@penzn
Copy link
Contributor Author

penzn commented Sep 3, 2019

Closing as we merged the alternative in #91

@penzn penzn closed this Sep 3, 2019
@penzn penzn deleted the unsigned-narrowing branch September 3, 2019 19:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants