Skip to content
This repository was archived by the owner on Apr 28, 2025. It is now read-only.

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
tgross35 pushed a commit that referenced this pull request Apr 18, 2025
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.

2 participants