-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Double Signaling NaN converted to float becomes INF #43252
Comments
Sorry, the initial shared code is wrong. Here is the correct one:
|
I can't reproduce this on macOS or Linux: Can you output LLVM IR as shown in the above link and paste it here? |
It works as expected on macOS and Linux. Only issue is with Windows. I am attaching the LLVM IR I just generated with the following command line: clang -g -o nan_d2f_clang.llvm -mllvm --x86-asm-syntax=intel -S -fcolor-diagnostics -fno-crash-diagnostics -O2 -g0 -emit-llvm nan_d2f.cpp |
LLVM IR clang -g -o nan_d2f_clang.llvm -mllvm --x86-asm-syntax=intel -S -fcolor-diagnostics -fno-crash-diagnostics -O2 -g0 -emit-llvm nan_d2f.cpp |
I don't know much about Windows, so someone else will have to investigate further. Here's the IR that I see in the attachment: define dso_local i32 @main() local_unnamed_addr #0 { declare dso_local i16 @_fdtest(float*) local_unnamed_addr #1 I've never seen "_fdtest" before... |
I reported the bug to Microsoft as it seems it is related to their STL. They were not able to reproduce the bug so I setup a repository in Azure DevOps to reproduce the bug. If that can help anyone to reproduce it I am sharing it here: https://dev.azure.com/feildel/clang-bug-nan-d2f |
Minimal repro: #include <assert.h> int main() { The assertion fires when compiled with clang-cl: clang version 9.0.0 (tags/RELEASE_900/final) but not for MSVC (or gcc/clang on Ubuntu, for that matter) per https://godbolt.org/z/65wWKG. |
In rust-lang/rust#69532 we've hit this bug, and I'm pretty confident I've figured out the issue. Here's a minimal example that just operates on IR https://godbolt.org/resetlayout/Zf7HMh , in case there's not one already in this thread. Here's a more illustrative test case that demonstrates what's happening: https://godbolt.org/z/c59E4E . It seems that the bottom 29 bits of the NaN payload are being discarded. I believe the issue is here: llvm-project/llvm/lib/Support/APFloat.cpp Lines 2203 to 2205 in 9868ea7
When I think LLVM has leeway with what to do precisely, but:
So, for the first part, I don't think IEEE 754 comes out an explicitly says that you should truncate the most significant bits when going from a higher-precision format to a lower precision one (I could be wrong), but it seems to imply it, at least for qNaN (sNaN is obviously implementation defined but it's hard for me to see why it should be treated very much differently to qNaN here):
That is, binary32 => binary64 => binary32 should be unchanged, which it would be for truncating the most significant bits, but not for shifting, or any other obvious method of discarding a subset of the bits. That said, this might just move your issue to the high bits unless you also ensure the output is a NaN if the input is one. I think this just needs to be an explicit check. I provided some rust code that does these things for rustc's port of APFloat in rust-lang/rust#69532 (comment) (it could be very wrong but probably gets the idea across — someone who works more regularly/deeply with IEEE 754 should take a look and possibly correct my mistakes — that's very welcome!). TL; DR:
P.S. I was filing this as a new issue when I noticed this at the last minute, which is why it's phrased like it would be a new issue. |
Is there some APFloat component that this can be assigned to? Would be good to get the APFloat maintainer(s) to look at this. |
I don't know if anyone is an APFloat maintainer/authority at this point, but I created patch proposal here: |
APFloat minimally fixed here: Can someone verify that the code on Windows behaves as expected now? We will hopefully fix this better with: |
The reported issue is fixed on Windows using clang & clang-cl built from commit e34bd1e. Thanks for your work! |
Will this fix make it to LLVM 11.0? Or shall we wait for LLVM 12? Also, I don't know if I should switch the status of the bug to resolved, or if some people are responsible for this. |
IIUC, this was not a recent regression, and the 11.0 release is close to done, but there's still a chance... |
If we have to spin another release candidate (rc5) we can pick up this fix, but otherwise it will have to wait for 11.0.1. I've made a note, but will remove it from the blockers list for now. |
There will be an rc5, so I've pushed this to 11.x as 60a2520. |
Great - thank you! |
mentioned in issue #46070 |
Extended Description
The following piece of code produce incorrect result with clang (and clang-cl) on Windows:
The program should return 1. It does on Linux with majors compilers. On Windows clang (and clang-cl) returns 0 no matter what compilation flags I tried. Here are some flags I used:
clang -O3 -Wall -Wextra -g nan_d2f.cpp
clang -O0 -Wall -Wextra -g nan_d2f.cpp
clang -Wall -Wextra -g nan_d2f.cpp
clang-cl /Ox /W4 /EHsc nan_d2f.cpp
clang-cl /W4 /EHsc nan_d2f.cpp
Here is version reported by
clang -v
:It was downloaded from here: https://releases.llvm.org/9.0.0/LLVM-9.0.0-win64.exe#/dl.7z
Let me know if you need more info or cannot reproduce.
The text was updated successfully, but these errors were encountered: