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

Fix inconsistent Intel hardware intrinsic APIs#19949

Merged
CarolEidt merged 2 commits into
dotnet:masterfrom
fiigii:fixhwapis
Sep 17, 2018
Merged

Fix inconsistent Intel hardware intrinsic APIs#19949
CarolEidt merged 2 commits into
dotnet:masterfrom
fiigii:fixhwapis

Conversation

@fiigii
Copy link
Copy Markdown

@fiigii fiigii commented Sep 13, 2018

  • Fixes https://github.com/dotnet/coreclr/issues/18831
  • Correct SSE2/AVX2 SumAbsoluteDifferences return type from Vector128<long/ulong> to Vector128<ushort>.
  • Rename SSE2 MultiplyHorizontalAdd to MultiplyAddAdjacent to keep consistent with SSSE3/AVX2 counterparts.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 13, 2018

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 13, 2018

If you guys think the API change is okay, I would disable their tests in this PR as well.

/// __m128i _mm_sad_epu8 (__m128i a, __m128i b)
/// PSADBW xmm, xmm/m128
/// </summary>
public static Vector128<long> SumAbsoluteDifferences(Vector128<byte> left, Vector128<byte> right) => SumAbsoluteDifferences(left, right);
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.

Shouldn't this be Vector128<ushort>?

Computes the absolute differences of the
packed unsigned byte integers from xmm2
/m128 and xmm1; the 8 low differences and 8
high differences are then summed separately
to produce two unsigned word integer results.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, right! Will fix.
Thank you!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Now they are correct and consistent with MultipleSumAbsoluteDifferences.

Copy link
Copy Markdown
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM.

Is this all of the renames/type fixes that we were tracking?

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 13, 2018

Yes, I am looking for more to make sure we fix all the inconsistency.

Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM

@BruceForstall
Copy link
Copy Markdown

@dotnet/dnceng Failure in https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_checked_ubuntu_innerloop_flow_prtest/7594/ is "java.io.IOException: No space left on device"

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 17, 2018

I checked that this kind of inconsistency is all fixed in this PR.
CI failure not related.

Can we merge?

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.

5 participants