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

Add a small conformance test to sqrt/sqrtf #274

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

ZagButNoZig
Copy link
Contributor

What this PR does

Add a test that guarantees that the result of sqrt/sqrtf is bit for bit identical on all platforms for a given array of test values.

Why this PR

This is a proposed fix for issue #255. Which asks all sqrt implementations to be IEEE 754 2008 compliant. While this does not prove compliance with the spec it at least proves conformance between platforms.

Issues with this PR

Since the values and their results are hard coded, they would need to be updated on every implementation change. The values are also somewhat arbitrary and don't cover every case inside the sqrt implementation.

Any feedback is very welcome.

@Amanieu
Copy link
Member

Amanieu commented Nov 15, 2022

Tests are failing for sqrt(-1) because the sign of the NaN is different from what was expected. I think this is an acceptable divergence.

@ZagButNoZig
Copy link
Contributor Author

I removed the tests with negative numbers. I'm really confused as to why the divergence happened in the first place. I have run the software sqrtf implementation on x86 (the non sse path) and it also seems to return 0xff800000, but as far as I can tell the ARM doesn't have a hardware intrinsic, so it must also be using the software implementation of sqrtf but returns a different result (0x7f800000). Which means both platforms produce a different result on this line if I didn't miss anything.

@Amanieu
Copy link
Member

Amanieu commented Nov 18, 2022

The sign of a NaN result is implementation-defined and not specified by IEEE-754. So it makes sense that different architectures produce different results.

@Amanieu Amanieu merged commit dc82800 into rust-lang:master Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants