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

[RISC-V] Test HalfTest: fixed payload preservation testcase for RISC-V #96888

Merged
merged 14 commits into from
Feb 5, 2024

Conversation

denis-paranichev
Copy link
Contributor

Arithmetic operations with NaN does not preserve payload on RISC-V. And explicit conversion operators in Half class use arithmetic operations with float. That is why payload preservation test cases are failed for RISC-V.

This fixes payload preservation testcases in:

  • System.Runtime.Tests.HalfTests.ExplicitConversion_FromSingle
  • System.Runtime.Tests.HalfTests.ExplicitConversion_ToSingle

Part of #84834
cc @tannergooding @gbalykov @t-mustafin @clamp03 @tomeksowi

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 12, 2024
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 12, 2024
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Jan 12, 2024
@denis-paranichev
Copy link
Contributor Author

@tannergoodin, I found that float->half and double->half cast algorithms are not similar. Is there a reason for such difference?

@tannergooding
Copy link
Member

Is there a reason for such difference?

The float->half version had a community member hand optimize it. The double->half version did not.

@denis-paranichev
Copy link
Contributor Author

@tannergooding, @jkotas, can this PR be merged?

@@ -1130,7 +1130,7 @@ public static void Equal(Half expected, Half actual, Half variance)
/// <exception cref="EqualException">Thrown when the representations are not identical</exception>
public static void Equal(double expected, double actual)
{
if (BitConverter.ToInt64(BitConverter.GetBytes(expected)) == BitConverter.ToInt64(BitConverter.GetBytes(actual)))
if (BitConverter.ToInt64(BitConverter.GetBytes(expected), 0) == BitConverter.ToInt64(BitConverter.GetBytes(actual), 0))
Copy link
Member

@tannergooding tannergooding Jan 31, 2024

Choose a reason for hiding this comment

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

This should be BitConverter.DoubleToInt64Bits(expected) == BitConverter.DoubleToInt64Bits(actual)

Copy link
Member

Choose a reason for hiding this comment

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

and similarly for the ones further down. For float on .NET Framework, I'd just define a helper:

static unsafe int SingleToInt32Bits(float value)
{
    return *(int*)&value;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thank you for suggestion. By the way I didn't find anything in the supported product list for the BitConverter.HalfToInt16Bits function in .NET API Catalog except the .NET version

Copy link
Member

Choose a reason for hiding this comment

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

The official API docs include the .NET versions its been included in: https://learn.microsoft.com/en-us/dotnet/api/system.bitconverter.halftoint16bits#applies-to

@denis-paranichev
Copy link
Contributor Author

@tannergooding, seems like the remaining CI failures are not caused by PR, could you review please?

@jkotas jkotas added area-System.Numerics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 5, 2024
@ghost
Copy link

ghost commented Feb 5, 2024

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Arithmetic operations with NaN does not preserve payload on RISC-V. And explicit conversion operators in Half class use arithmetic operations with float. That is why payload preservation test cases are failed for RISC-V.

This fixes payload preservation testcases in:

  • System.Runtime.Tests.HalfTests.ExplicitConversion_FromSingle
  • System.Runtime.Tests.HalfTests.ExplicitConversion_ToSingle

Part of #84834
cc @tannergooding @gbalykov @t-mustafin @clamp03 @tomeksowi

Author: denis-paranichev
Assignees: denis-paranichev
Labels:

area-System.Numerics, community-contribution, arch-riscv, needs-area-label

Milestone: -

@tannergooding tannergooding merged commit bb2376c into dotnet:main Feb 5, 2024
107 of 111 checks passed
@denis-paranichev
Copy link
Contributor Author

Thank you all for review!
@tannergooding, thank you for merge it!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants