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 TotalOrderIeee754ComparerTests: Fix NFloat - NaN testcases for RISC-V #97340

Merged

Conversation

denis-paranichev
Copy link
Contributor

float->double cast instruction produced by NFloat constructor does not preserve NaN sign and payload on RISC-V.
That's why all NaN values in test data are same positive NaNs without payload.

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

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 22, 2024
@ghost
Copy link

ghost commented Jan 22, 2024

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

Issue Details

float->double cast instruction produced by NFloat constructor does not preserve NaN sign and payload on RISC-V.
That's why all NaN values in test data are same positive NaNs without payload.

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

Author: denis-paranichev
Assignees: -
Labels:

area-System.Numerics

Milestone: -

@denis-paranichev
Copy link
Contributor Author

@dotnet-policy-service agree company="Samsung"

Comment on lines 110 to 116
if (PlatformDetection.IsRiscV64Process) // float->double cast does not preserve NaN payload and sign on RISC-V
{
yield return new object[] { BitConverter.UInt32BitsToSingle(0xFFC00000), float.NegativeInfinity, 1 };
yield return new object[] { BitConverter.UInt32BitsToSingle(0xFFC00000), -1.0f, 1 };
yield return new object[] { BitConverter.UInt32BitsToSingle(0xFFC00000), BitConverter.UInt32BitsToSingle(0x7FC00000), 0 };
yield return new object[] { BitConverter.UInt32BitsToSingle(0x7FC00000), BitConverter.UInt32BitsToSingle(0x7FC00001), 0 }; // implementation defined, not part of IEEE 754 totalOrder
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the desirable/best fix.

The overall preservation of a NaN sign/payload is implementation defined; however, in the face of a NaN with sign and/or payload, IEEE 754 totalOrder describes exact handling/behavior that is required to be followed.

The test was using SingleTestData for simplicity purposes; however in the face of RiscV it is likely better to fix the test to take NFloat and to enumerate bitwise correct values depending on the underlying bitness of the platform. That way encountered NaN with sign/payload (such as from the developer doing a bitcast where RiscV doesn't normalize) can be validated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to enumerate bitwise correct values only for NaN values or for all test values?

@tannergooding
Copy link
Member

Looks like there are some CI failures related to this PR that need to be resolved still, specifically related to NFloat..

Failures look to be similar to System.ArgumentException : Object of type 'System.Double' cannot be converted to type 'System.Runtime.InteropServices.NFloat'. and so I expect you need to ensure the test data pre-casts the literal to NFloat

@denis-paranichev
Copy link
Contributor Author

Looks like there are some CI failures related to this PR that need to be resolved still, specifically related to NFloat..

Failures look to be similar to System.ArgumentException : Object of type 'System.Double' cannot be converted to type 'System.Runtime.InteropServices.NFloat'. and so I expect you need to ensure the test data pre-casts the literal to NFloat

Thank you for noticing, I have fixed this failure.

@tannergooding tannergooding merged commit 541857c into dotnet:main Feb 1, 2024
111 checks passed
@denis-paranichev
Copy link
Contributor Author

@tannergooding, thank you for merge it!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 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.

7 participants