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
tannergooding wants to merge 1 commit into
dotnet:masterfrom
tannergooding:missing-intrinsics
Closed

Adding missing overloads of the Sse2 and Sse41 Extract methods#21197
tannergooding wants to merge 1 commit into
dotnet:masterfrom
tannergooding:missing-intrinsics

Conversation

@tannergooding
Copy link
Copy Markdown
Member

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

@tannergooding
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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