Skip to content

WIP: Don't assume all NaNs being type-converted are quiet NaNs #952

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

Closed
wants to merge 1 commit into from

Conversation

lewis1revill
Copy link

When truncating floating point types, we detect whether the original value was any kind of NaN, but reconstruct the destination value with the qNaN bit always set - meaning any signalling NaNs get lost. This patch explicitly includes the source qNaN bit as part of the data which is copied over.

When truncating floating point types, we detect whether the original
value was any kind of NaN, but reconstruct the destination value with
the qNaN bit always set - meaning any signalling NaNs get lost. This
patch explicitly includes the source qNaN bit as part of the data which
is copied over.
@lewis1revill
Copy link
Author

WIP, need to add testing and also get some feedback on whether the fix should look any different to this

@quaternic
Copy link
Contributor

If the payload would be 0 after dropping the low-order bits, the result cannot be represented as a signalling NaN, since that is the representation for infinity.

In my opinion, always quieting is the correct behavior.

@beetrees
Copy link
Contributor

Rust NaN semantics allow float type conversions to quieten signalling NaNs (for reference, the IEEE 754 standard actually requires this quietening), so this isn't a bug.

@lewis1revill
Copy link
Author

If the payload would be 0 after dropping the low-order bits, the result cannot be represented as a signalling NaN, since that is the representation for infinity.

In my opinion, always quieting is the correct behavior.

Thanks, I can understand that would be sensible (possibly what's causing the CI failures? I haven't tried to reproduce them).

We came across this as an issue when running the test library/std/floats/fp16.rs::test_total_cmp for our custom target, for which the legal type is i32. Type legalization in total_cmp caused the bitcast between f16 and i16 to actually be an fp_to_fp32 node followed directly by an fp32_to_fp (which is totally valid, and it only occurs with unoptimized codegen), and so when the input was a signalling NaN, that round trip was actually turning it into a quiet NaN, so the failure occurred. Without a fix similar to this, it's difficult to think of a way to make that test pass.

@quaternic
Copy link
Contributor

Seems related to:
LLVM bug: llvm/llvm-project#104915 (comment)
Some snan tests have been temporarily disabled in rust-lang/rust@19fd098

@beetrees
Copy link
Contributor

Type legalization in total_cmp caused the bitcast between f16 and i16 to actually be an fp_to_fp32 node followed directly by an fp32_to_fp (which is totally valid, and it only occurs with unoptimized codegen)

As you've discovered, this isn't a valid legalization as it can quieten signalling NaNs. It sounds like you're encountering llvm/llvm-project#104915 (comment) as quaternic mentioned. Since you've mentioned you're using a custom target, note that some less common architectures still use the broken TypePromoteFloat, which has even bigger issues that are tracked in llvm/llvm-project#97975.

@lewis1revill
Copy link
Author

Seems related to: LLVM bug: llvm/llvm-project#104915 (comment) Some snan tests have been temporarily disabled in rust-lang/rust@19fd098

Thanks, that's exactly it. I will close this PR then and keep an eye on the LLVM issue

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.

3 participants