Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Adding missing overloads of the Sse2 and Sse41 Extract methods #21197

Closed
wants to merge 1 commit into from
Closed

Adding missing overloads of the Sse2 and Sse41 Extract methods #21197

wants to merge 1 commit into from

Conversation

tannergooding
Copy link
Member

We were missing the signed overloads of a few of these methods.

@tannergooding
Copy link
Member Author

CC. @fiigii, @CarolEidt, @eerhardt

/// int _mm_extract_epi8 (__m128i a, const int imm8)
/// PEXTRB reg/m8, xmm, imm8
/// </summary>
public static byte Extract(Vector128<sbyte> value, byte index) => Extract(value, index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be public static sbyte Extract?

@fiigii
Copy link

fiigii commented Nov 26, 2018

This is intentional #17637

/// int _mm_extract_epi8 (__m128i a, const int imm8)
/// PEXTRB reg/m8, xmm, imm8
/// </summary>
public static byte Extract(Vector128<sbyte> value, byte index) => Extract(value, index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(super nit) - all the other overloads has the "signed" version listed first, then the "unsigned" version. Do we want to keep it consistent to have the signed version first here?

@tannergooding
Copy link
Member Author

This is intentional

Ah, right. I forgot about that.... I encountered this when trying to implement the GetElement methods and it is definitely confusing given that we do have these overloads for Insert...

@fiigii
Copy link

fiigii commented Nov 26, 2018

it is definitely confusing given that we do have these overloads for Insert...

Yes, but it is caused by the hardware feature...

@tannergooding
Copy link
Member Author

Yes, but it is caused by the hardware feature...

Right. The current issue is that PEXTRB and PEXTRW always zero-extend. I think, if nothing else, we need comments here in the source indicating that we don't include these and why....

However, we should probably also explicitly discuss and get an official API decision (as we've done with the other cases so far) on what we want to do here:

  • Don't include the APIs
  • Include them and return the unsigned type
  • Include them and return the signed type, zero-extending
  • Include them and return the signed type, explicitly sign-extending where necessary

@tannergooding
Copy link
Member Author

Closing this and opened https://github.com/dotnet/corefx/issues/33696

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