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

Don't mask NaN's, or make it a configuration option? #103

Closed
Gankra opened this issue Nov 13, 2017 · 6 comments
Closed

Don't mask NaN's, or make it a configuration option? #103

Gankra opened this issue Nov 13, 2017 · 6 comments

Comments

@Gankra
Copy link
Contributor

Gankra commented Nov 13, 2017

Noticed this while looking at the codegen for deserializing a 4x4 f32 matrix with bincode, which otherwise should be a memcopy.

Having gone through the history of how this happened, the LLVM concerns are all bogus, it handles sNaN fine. I don't have much of an opinion on the platform-specific stuff, but at very least this seems like a reasonable feature-flag, or even target cfg thing. Certainly for my use-case, this mask does nothing.

@BurntSushi
Copy link
Owner

BurntSushi commented Nov 13, 2017 via email

@Gankra
Copy link
Contributor Author

Gankra commented Nov 13, 2017

I discussed it with @regehr and some others who work on llvm, and they all agreed it was just bad docs using ancient C-semantics rather than llvm semantics (C vaguely tries to support non-IEEE floats, but LLVM doesn't). The same docs also say fdiv %X, 0 is Undefined Behaviour, which is obviously false.

I am working on upstreaming a patch to change the problematic docs (which are just trying to explain undef/UB, and actually have nothing to do with defining fdiv): Gankra/llvm@850f0ae

The docs that caused the scare, for reference:

screen shot 2017-11-10 at 10 40 39 pm

And the actual docs for fdiv indicate there's no inputs that cause trouble:

screen shot 2017-11-12 at 10 00 56 pm

If there were any issues, the Semantics section would note them, as the udiv instruction does:

screen shot 2017-11-12 at 10 05 53 pm

@BurntSushi
Copy link
Owner

BurntSushi commented Nov 13, 2017 via email

@Gankra
Copy link
Contributor Author

Gankra commented Nov 13, 2017

Ah, I wasn't going to try to fix std since from_bits was stabilized documenting this behaviour, but I see now that it was a "may", so we can just change the impl without touching the documented contract.

@BurntSushi
Copy link
Owner

BurntSushi commented Nov 13, 2017 via email

@BurntSushi
Copy link
Owner

byteorder 1.2.0 is on crates.io with this bug fixed.

BurntSushi pushed a commit to diffeo/kodama that referenced this issue Nov 29, 2017
It turns out that masking signaling NaN isn't necessary to avoid UB,
which in turn meant that the byteorder crate could remove some unnecessary
uses of unsafe, which lets us remove them as well.

See: BurntSushi/byteorder#103
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

No branches or pull requests

2 participants