-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
Comments
You can't just say "the LLVM concerns are bogus." Can you show it?
On Nov 12, 2017 21:36, "Alexis Beingessner" <notifications@github.com> wrote:
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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#103>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAb34ssDC0YLCwaKPvgT9KcNLmxo7HH8ks5s16tIgaJpZM4QbK6e>
.
|
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 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: And the actual docs for fdiv indicate there's no inputs that cause trouble: If there were any issues, the Semantics section would note them, as the udiv instruction does: |
This should also be brought up with std as well, since from_bits has the
same masking behavior.
…On Nov 12, 2017 22:05, "Alexis Beingessner" ***@***.***> wrote:
I discussed it with @regehr <https://github.com/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:
[image: screen shot 2017-11-10 at 10 40 39 pm]
<https://user-images.githubusercontent.com/1136864/32707850-c15c96c8-c7f4-11e7-8f30-1dda92b91627.png>
And the *actual* docs for fdiv indicate there's no inputs that cause
trouble:
[image: screen shot 2017-11-12 at 10 00 56 pm]
<https://user-images.githubusercontent.com/1136864/32707919-2b680570-c7f5-11e7-8411-d2d30193c68d.png>
If there were any issues, the Semantics section would note them, as the
udiv instruction does:
[image: screen shot 2017-11-12 at 10 03 13 pm]
<https://user-images.githubusercontent.com/1136864/32707973-788c4f14-c7f5-11e7-8fc5-fad49e3bb8ae.png>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAb34rfZU3c6YfjiuBWgBmkapwE0HPHqks5s17HkgaJpZM4QbK6e>
.
|
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. |
Exactly. That "may" was very intentional because we were very unsure of
ourselves, so we took the conservative route to leave it open to being
fixed for good. :)
…On Nov 12, 2017 22:34, "Alexis Beingessner" ***@***.***> wrote:
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.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAb34iP_zkcV1sLQOPvfLdDKA1U-mbh_ks5s17iugaJpZM4QbK6e>
.
|
|
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
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.
The text was updated successfully, but these errors were encountered: