Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CompareGreaterThan, CompareGreaterThanOrEqual, CompareNotGreaterThan, and CompareNotGreaterThanOrEqual have incorrect implementations #34094

Closed
tannergooding opened this issue Mar 25, 2020 · 2 comments · Fixed by #34204
Labels
arch-x64 area-System.Runtime.Intrinsics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug

Comments

@tannergooding
Copy link
Member

As per the title, the following methods have incorrect implementations and won't handle NaN inputs correctly:

  • NI_SSE_CompareGreaterThan
  • NI_SSE_CompareScalarGreaterThan
  • NI_SSE_CompareGreaterThanOrEqual
  • NI_SSE_CompareScalarGreaterThanOrEqual
  • NI_SSE_CompareNotGreaterThan
  • NI_SSE_CompareScalarNotGreaterThan
  • NI_SSE_CompareNotGreaterThanOrEqual
  • NI_SSE_CompareScalarNotGreaterThanOrEqual
  • NI_SSE2_CompareGreaterThan
  • NI_SSE2_CompareScalarGreaterThan
  • NI_SSE2_CompareGreaterThanOrEqual
  • NI_SSE2_CompareScalarGreaterThanOrEqual
  • NI_SSE2_CompareNotGreaterThan
  • NI_SSE2_CompareScalarNotGreaterThan
  • NI_SSE2_CompareNotGreaterThanOrEqual
  • NI_SSE2_CompareScalarNotGreaterThanOrEqual

For pre-AVX hardware, there isn't a hardware comparison mode that performs the given operations and so they are currently being implemented using the inverse operation.
However, using the inverse operation (such as converting GreaterThan to NotLessThanOrEqual) does not correctly handle the unordered relation (when one or both inputs is NaN).

These should instead be using a different inverse relation and swapping the operands. For example, CompareGreaterThan(a, b) should be emitted as CompareLessThan(b, a) as native does and as recommended by the x86 instruction manual: https://godbolt.org/z/ghwY5N

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 25, 2020
@tannergooding
Copy link
Member Author

I found this while implementing #31193.

CC. @CarolEidt, @echesakovMSFT

@tannergooding
Copy link
Member Author

This would be a technically breaking change to fix, but one we should likely take. Especially given the difference in behavior users will see for the 256-bit overloads being added in #31193 and people using the FloatComparisonMode enum overload.

@tannergooding tannergooding added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed untriaged New issue has not been triaged by the area owner labels Mar 27, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x64 area-System.Runtime.Intrinsics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants