Skip to content

u64 to f32 conversion is inaccurate on Windows 32bit #59787

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
ghost opened this issue Jan 2, 2023 · 10 comments
Closed

u64 to f32 conversion is inaccurate on Windows 32bit #59787

ghost opened this issue Jan 2, 2023 · 10 comments

Comments

@ghost
Copy link

ghost commented Jan 2, 2023

On Windows, the x87 control word has a different default value than on other platforms. This not only introduces floating point precision issues, but also results in inconsistent behavior with other platforms. I tried to avoid using x87 by adding the -msse2 option when compiling for Windows 32bit, but the compiler still generates binaries using only x87.

The code below outputs differently on Windows 32bit than on other platforms:

#include <stdio.h>
#include <stdint.h>

int main() {
    int64_t n = 0b0000000000111111111111111111111111011111111111111111111111111111;
    printf("%lld, %.0f, %.0f", n, (float)n, (float)(uint64_t)n);

    return 0;
}

rust-lang/rust#105626

@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2023

@llvm/issue-subscribers-backend-x86

@topperc
Copy link
Collaborator

topperc commented Jan 3, 2023

Here's a patch to use library call for Windows instead of the broken inline expansion.

diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index c0aeed8e45fb..adf236d5d7d0 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -21880,6 +21880,11 @@ SDValue X86TargetLowering::LowerUINT_TO_FP(SDValue Op,
   }
 
   assert(SrcVT == MVT::i64 && "Unexpected type in UINT_TO_FP");
+
+  // Windows default precision control does not support 80-bit FP.
+  if (Subtarget.isOSWindows())
+    return SDValue();
+
   SDValue ValueToStore = Src;
   if (isScalarFPTypeInSSEReg(Op.getValueType()) && !Subtarget.is64Bit()) {
     // Bitcasting to f64 here allows us to do a single 64-bit store from

@ghost ghost changed the title x87 control word is different on Windows 32bit x87 control word is different on Windows Jan 3, 2023
@ghost
Copy link
Author

ghost commented Jan 3, 2023

I also tried clang.exe test.c -mno-x87 -msse -msse2 but it still only uses x87. I tested it with LLVM for Windows 32bit.

@Alcaro
Copy link
Contributor

Alcaro commented Jan 3, 2023

I also tried clang.exe test.c -mno-x87 -msse -msse2 but it still only uses x87. I tested it with LLVM for Windows 32bit.

-mfpmath=sse

No clue why -msse/-msse2 doesn't imply that. It doesn't affect ABI, it still passes arguments on stack and return in x87.

@topperc
Copy link
Collaborator

topperc commented Jan 3, 2023

On Windows 32bit, I also tried clang.exe test.c -mno-x87 -msse -msse2 but it still only uses x87.

-mno-x87 is not correctly implemented and is poorly tested. It was added after a lot of code was written and a lot of that hasn’t been updated.

There’s no sse2 instruction that can do unsigned 64-bit integer to float on a 32 bit target. So we use x87. But the algorithm only works if x87 is configured for full precision. If not it rounds incorrectly.

@phoebewang
Copy link
Contributor

MSVC has the same problem: https://godbolt.org/z/4sbn8dq1T

@ghost ghost changed the title x87 control word is different on Windows u64 to f32 conversion is inaccurate in Windows 32bit Jan 3, 2023
@ghost
Copy link
Author

ghost commented Jan 13, 2023

MSVC has the same problem: https://godbolt.org/z/4sbn8dq1T

MSVC x86 does not seem to produce different results depending on whether or not it has sign, so it is less likely to be an issue. It might be better to just match MSVC's behavior than to use library calls, even though we lose a tiny bit of precision.

@ghost
Copy link
Author

ghost commented Jan 19, 2023

Fixed in a6e3027

@ghost ghost closed this as completed Jan 19, 2023
@topperc topperc reopened this Jan 19, 2023
@topperc
Copy link
Collaborator

topperc commented Jan 19, 2023

I'm going to revert the patch. It broke Chrome and Halide builds. I'm going to look at modifying PC in FPCW on the fly.

@ghost
Copy link
Author

ghost commented Feb 7, 2023

Fixed again in 11fb09e

@ghost ghost closed this as completed Feb 7, 2023
@ghost ghost changed the title u64 to f32 conversion is inaccurate in Windows 32bit u64 to f32 conversion is inaccurate on Windows 32bit Dec 14, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants