Skip to content

Inconsistent spilling of floating point causes simple floating point comparison to fail in 32 bit #42470

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
ldionne opened this issue Aug 27, 2019 · 4 comments
Labels
bugzilla Issues migrated from bugzilla clang:codegen IR generation bugs: mangling, exceptions, etc. invalid Resolved as invalid, i.e. not a bug

Comments

@ldionne
Copy link
Member

ldionne commented Aug 27, 2019

Bugzilla Link 43125
Version trunk
OS All
CC @efriedma-quic,@zygoloid

Extended Description

In the following code, the assertion fails when compiled for and run in 32 bit mode:

$ cat <<EOF | clang++ -xc++ - -m32 -std=c++11 && ./a.out

#include <cassert>
#include <cstdint>

uint_fast32_t min() { return 1; }
uint_fast32_t max() { return 2147483646; }

int main() {
    uint_fast32_t x = max() - min();
    double y = 2100000000.0;
    assert((x * y) == (max() - min()) * y);
}
EOF

Here, notice that both the right hand side and the left hand side of the comparison are "the same", the only difference being that in the left hand side, we cached the value of max() - min() into a local variable (x). When storing both the left hand side and the right hand side into named variables, the comparison succeeds:

$ cat <<EOF | clang++ -xc++ - -m32 -std=c++11 && ./a.out

#include <cassert>
#include <cstdint>

uint_fast32_t min() { return 1; }
uint_fast32_t max() { return 2147483646; }

int main() {
    uint_fast32_t x = max() - min();
    double y = 2100000000.0;
    double left = x * y;
    double right = (max() - min()) * y;
    assert(left == right);
}
EOF

This only reproduces in 32 bit mode. Howard Hinnant hypothesized this was caused by the compiler storing one in a register and the other one on the stack, when the register might not be able to hold a true IEEE 64 bit double.

I know comparing floating point values is normally frowned upon, so feel free to close this as "You shouldn't be doing this". However, since the calculation is incredibly simple and the difference between the right and the left hand sides is just "stashing to a variable", I thought this might still be considered a bug.

@efriedma-quic
Copy link
Collaborator

x87 floating point is weird: even though you wrote "double" in the source code, it only has 80-bit operations. Given that, we basically have the following options:

  1. Turn a simple arithmetic operation into some complex series of operations that gets rounded correctly. (IIRC, if you modify the control word, and store to the stack after every operation, you can synthesize correctly rounded operations.)

  2. Store to the stack after every operation so we get consistent results. This is gcc's -ffloat-store. This doesn't round correctly.

  3. Pretend that the long double registers are actually double, and throw the obscure rounding problems at the user in exchange for a performance boost. This is what clang and gcc do by default.


We could theoretically do something special specifically for equality comparisons, but I'm not sure that's worthwhile.

If you build with "-msse2", you get faster code that avoids these issues.

@ldionne
Copy link
Member Author

ldionne commented Aug 29, 2019

Okay, so I'm taking this as: "really don't ever compare floats for equality, unless you know EXACTLY what you're doing". Is this what my conclusion should be? I'd be fine with that.

If so, feel free to close. This came up in the context of fixing a libc++ test which has been fixed since anyway, so I was filing this bug report for completeness to make sure we were not leaving an important bug on the table.

@ldionne
Copy link
Member Author

ldionne commented Sep 25, 2019

Ping -- should I close this as "I didn't know what I was doing"?

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@arsenm arsenm added the invalid Resolved as invalid, i.e. not a bug label Aug 13, 2023
@arsenm
Copy link
Contributor

arsenm commented Aug 13, 2023

It's basically impossible to get consistent results out of x87 without always forcing write to memory

@arsenm arsenm closed this as completed Aug 13, 2023
@EugeneZelenko EugeneZelenko closed this as not planned Won't fix, can't repro, duplicate, stale Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang:codegen IR generation bugs: mangling, exceptions, etc. invalid Resolved as invalid, i.e. not a bug
Projects
None yet
Development

No branches or pull requests

4 participants