[X86] Avoid converting u64 to f32 using x87 on Windows

[icedrocket]

Summary

The code below currently prints less accurate values only on Windows 32-bit. On Windows, the default precision control on x87 is only 53-bit, and FADD triggers rounding with that precision, so the final result may be less accurate. This revision avoids less accurate conversions by using library calls instead.

#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;
}

[lebedev.ri]

Please upload all patches with full context (-U99999)
Tests missing

[lebedev.ri]

only on Windows 32-bit

Is the default precision control different for 64-bit?

[craig.topper]

In D141074#4029537, @lebedev.ri wrote:

only on Windows 32-bit

Is the default precision control different for 64-bit?

No, but I think we use a different algorithm with a 64-bit cvtsi2ss on 64-bit targets so we don't end up using x87.

[lebedev.ri]

In D141074#4029541, @craig.topper wrote:

In D141074#4029537, @lebedev.ri wrote:

only on Windows 32-bit

Is the default precision control different for 64-bit?

No, but I think we use a different algorithm with a 64-bit cvtsi2ss on 64-bit targets so we don't end up using x87.

I'm mainly asking whether this needs to check for the 32-bit windows specifically, not just that it is windows. Guess not.

[icedrocket]

In D141074#4029544, @lebedev.ri wrote:

In D141074#4029541, @craig.topper wrote:

In D141074#4029537, @lebedev.ri wrote:

only on Windows 32-bit

Is the default precision control different for 64-bit?

No, but I think we use a different algorithm with a 64-bit cvtsi2ss on 64-bit targets so we don't end up using x87.

I'm mainly asking whether this needs to check for the 32-bit windows specifically, not just that it is windows. Guess not.

I tested it on my machine and the precision control is 53-bit even on Windows 64-bit.

[pengfei]

llvm/lib/Target/X86/X86ISelLowering.cpp

21373

What to do if user changes the default precision? Besides, our down stream offers option to help user to set higher percision.

[icedrocket]

This comment was removed by icedrocket.

[icedrocket]

What to do if user changes the default precision?

If users change the precision control to 64-bit, we can allow the use of x87 instructions by adding a compiler option. However, this is probably only for Windows 32-bit, and I don't know if it's appropriate to add a new option because there aren't that many users on that platform.

Besides, our down stream offers option to help user to set higher percision.

I'm sorry, but I'm not sure exactly what that option is. Is it an option to increase x87 precision, or an option to operate on floats at higher precision like double?

[icedrocket]

Use library calls only when SSE is enabled. If SSE is disabled and x87 is enabled, the x87 implementation is used regardless of precision.

[pengfei]

In D141074#4031117, @icedrocket wrote:

What to do if user changes the default precision?

If users change the precision control to 64-bit, we can allow the use of x87 instructions by adding a compiler option. However, this is probably only for Windows 32-bit, and I don't know if it's appropriate to add a new option because there aren't that many users on that platform.

Besides, our down stream offers option to help user to set higher percision.

I'm sorry, but I'm not sure exactly what that option is. Is it an option to increase x87 precision, or an option to operate on floats at higher precision like double?

It's /Qpc80 that to be compatible with ICC https://www.intel.com/content/www/us/en/develop/documentation/cpp-compiler-developer-guide-and-reference/top/optimization-and-programming/intel-c-compiler-classic-math-library/use-the-intel-c-compiler-classic-math-library.html
It's intended design for Windows, especially for 32-bits.

[icedrocket]

Add the pc80 option. There may be other suitable candidate names for this option, but I used the name from Intel ICC. The current implementation only applies to conversions from 64-bit integers to floating point.

[craig.topper]

In D141074#4031999, @icedrocket wrote:

Add the pc80 option. There may be other suitable candidate names for this option, but I used the name from Intel ICC. The current implementation only applies to conversions from 64-bit integers to floating point.

The icc options injects code into main to set the PC. (Kind of ignoring that global constructors that run before main exist).

[icedrocket]

In D141074#4032006, @craig.topper wrote:

In D141074#4031999, @icedrocket wrote:

Add the pc80 option. There may be other suitable candidate names for this option, but I used the name from Intel ICC. The current implementation only applies to conversions from 64-bit integers to floating point.

The icc options injects code into main to set the PC. (Kind of ignoring that global constructors that run before main exist).

The option I added just tells the compiler that the precision control will always be set to 64-bit. And it's still a draft implementation of the candidate concept.

[icedrocket]

On Windows 32-bit, the calling convention uses x87, so it's almost impossible to avoid using x87. So rather than avoiding the use of x87, I think it's better to add an option to inject code like an ICC option. To add this option to all LLVM-based compilers, the code must be injected into the LLVM IR. This will be done in the process of parsing or generating the LLVM IR. However, this requires refactoring a lot of code.

[icedrocket]

This comment was removed by icedrocket.

[pengfei]

I think it's not a good idea to use the same name while not doing the same thing as ICC. I considered to open source our implementation if people thinks it's useful, though I may not do it immediately since it relys on some internal framework.
Can we just emit a warning to remind user the result may not be precise on 32-bit Windows. I think the current implementation still has two problems: 1) The result is not identical to MSVC's, which may introduce more problem than it solves since MSVC is the dominant compiler on Windows. 2) The generated binary relys on compiler-rt, but I think not all user will use compiler-rt. Force to generate library call may result in compiler fail to them.

[lebedev.ri]

I think the original version of the diff is reasonable and can be merged.

I don't think the concerns that we lower something to a libcall are sufficiently grounded.
We already lower many things to a libcall. Doing so for one more thing is not the end of the world,
assuming the compiler-rt exists for that configuration in the first place.

[icedrocket]

In D141074#4037440, @lebedev.ri wrote:

I think the original version of the diff is reasonable and can be merged.

I don't think the concerns that we lower something to a libcall are sufficiently grounded.
We already lower many things to a libcall. Doing so for one more thing is not the end of the world,
assuming the compiler-rt exists for that configuration in the first place.

I've written code that adds a warning that the results may be incorrect if x87 is used on Windows 32-bit. Would it be a good idea to add this code too?

[lebedev.ri]

Does the libcall not produce the right answer? That sounds like a bug.
I think this patch should *just* be about falling back to the libcall.

[icedrocket]

If SSE2 is available, we only need to use library calls to convert between 64-bit integers and floating point. However, if only x87 is available, precision issues are unavoidable. What I meant was to just use the x87 implementation and print a warning in this case.

[craig.topper]

In D141074#4037831, @icedrocket wrote:

If SSE2 is available, we only need to use library calls to convert between 64-bit integers and floating point. However, if only x87 is available, precision issues are unavoidable. In this case, it might be better to just use the x87 implementation and print a warning.

Hopefully if you're not using long double, there shouldn't be any major precision issues with x87. The issue with the unsigned conversion is assuming we could represent a 64-bit integer in the mantissa of an x87 register without occurring any rounding.

[lebedev.ri]

In D141074#4037831, @icedrocket wrote:

If SSE2 is available, we only need to use library calls to convert between 64-bit integers and floating point. However, if only x87 is available, precision issues are unavoidable. What I meant was to just use the x87 implementation and print a warning in this case.

Why are they unavoidable?

[icedrocket]

In D141074#4037854, @lebedev.ri wrote:

In D141074#4037831, @icedrocket wrote:

If SSE2 is available, we only need to use library calls to convert between 64-bit integers and floating point. However, if only x87 is available, precision issues are unavoidable. What I meant was to just use the x87 implementation and print a warning in this case.

Why are they unavoidable?

As long as the precision control is low, x87 has low precision.

[craig.topper]

In D141074#4037902, @icedrocket wrote:

In D141074#4037854, @lebedev.ri wrote:

In D141074#4037831, @icedrocket wrote:

If SSE2 is available, we only need to use library calls to convert between 64-bit integers and floating point. However, if only x87 is available, precision issues are unavoidable. What I meant was to just use the x87 implementation and print a warning in this case.

Why are they unavoidable?

As long as the precision control is low, x87 has low precision.

If the precision control is set to more than 24-bit precision, x87 actually has more precision than SSE for fp32.

[lebedev.ri]

Please split clang change into a separate review.

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

21373

Where did 56 come from? The precision control settings are 24, 53, and 64.

[icedrocket]

This comment was removed by icedrocket.

[icedrocket]

Fix the comments

[craig.topper]

Tests?

[icedrocket]

The test keeps failing, but Diff 486869 has already built and tested successfully. The only thing that has changed is the comments.

[icedrocket]

This comment was removed by icedrocket.

[icedrocket]

Change to original version of diff mentioned by @lebedev.ri.

[craig.topper]

Can we add lit test to llvm/test/CodeGen/X86 for this with a Windows triple?

[icedrocket]

The main reason I posted a review was because the conversion results from u64 and i64 to f32 did not match, causing problems.

I think using the x87 implementation of UINT_TO_FP also in i64 to f32 conversion is an alternative solution without compromising performance on Windows 32-bit. This will produces same results as MSVC and solves the problem that prompted me to post this review. There may be a very slight drop in precision, but it shouldn't be a major issue.

[icedrocket]

I've checked and MSVC's implementation dynamically checks if AVX-512 is available and uses vcvtuqq2pd if available. Therefore, it seems that the result depends on whether the system supports AVX-512 or not.

[icedrocket]

The tests keep failing, but it's all RISC-V related, so the diff probably isn't wrong.

[icedrocket]

I'm not sure why the result depends on the x87 control word, but anyway, there is a possibility that this issue is because of the rounding mode. The conversion from f64 to f32 is typically done using round to nearest, and the target to round can differ between u64 and u64 to f64. We should round over u64, not u64 to f64.

[craig.topper]

In D141074#4054355, @icedrocket wrote:

I'm not sure why the result depends on the x87 control word, but anyway, there is a possibility that this issue is because of the rounding mode. The conversion from f64 to f32 is typically done using round to nearest, and the target to round can differ between u64 and u64 to f64. We should round over u64, not u64 to f64.

The algorithm we use for u64 to f32 looks like this

bitcast u64 to i64
convert to i64 to 80-bit fp using FILD instruction (this conversion does not depend on the value of PC).
if bits 63 was set in the integer this conversion produces a negative number.
if bit 63 was set in the integer, add 18446744073709551616 to the negative floating point value to give it the correct positive value. If bit 63 wasn’t set, add 0.0. With PC=64 this shouldn't result in any rounding.
round 80-bit fp value to f32 using FST.

The bug occurs because the addition of 18446744073709551616 ends up rounding unexpectedly because of PC=53.

[icedrocket]

In D141074#4054359, @craig.topper wrote:

In D141074#4054355, @icedrocket wrote:

I'm not sure why the result depends on the x87 control word, but anyway, there is a possibility that this issue is because of the rounding mode. The conversion from f64 to f32 is typically done using round to nearest, and the target to round can differ between u64 and u64 to f64. We should round over u64, not u64 to f64.

The algorithm we use for u64 to f32 looks like this

bitcast u64 to i64
convert to i64 to 80-bit fp using FILD instruction (this conversion does not depend on the value of PC).
if bits 63 was set in the integer this conversion produces a negative number.
if bit 63 was set in the integer, add 18446744073709551616 to the negative floating point value to give it the correct positive value. With PC=64 this shouldn't result in any rounding.
round 80-bit fp value to f32 using FST.

The bug occurs because the addition of 18446744073709551616 ends up rounding unexpectedly because of PC=53.

The value I tried to convert in summary's example code is 18014397972611071, which is representable in i64 range and bit 63 is not set.

[craig.topper]

In D141074#4054383, @icedrocket wrote:

In D141074#4054359, @craig.topper wrote:

In D141074#4054355, @icedrocket wrote:

I'm not sure why the result depends on the x87 control word, but anyway, there is a possibility that this issue is because of the rounding mode. The conversion from f64 to f32 is typically done using round to nearest, and the target to round can differ between u64 and u64 to f64. We should round over u64, not u64 to f64.

The algorithm we use for u64 to f32 looks like this

bitcast u64 to i64
convert to i64 to 80-bit fp using FILD instruction (this conversion does not depend on the value of PC).
if bits 63 was set in the integer this conversion produces a negative number.
if bit 63 was set in the integer, add 18446744073709551616 to the negative floating point value to give it the correct positive value. With PC=64 this shouldn't result in any rounding.
round 80-bit fp value to f32 using FST.

The bug occurs because the addition of 18446744073709551616 ends up rounding unexpectedly because of PC=53.

The value I tried to convert in summary's example code is 18014397972611071, which is representable in i64 range and bit 63 is not set.

If bit 63 isn’t set we add 0.0 which still triggers the rounding to 53 bits on the add.

[icedrocket]

In D141074#4054355, @icedrocket wrote:

I'm not sure why the result depends on the x87 control word, but anyway, there is a possibility that this issue is because of the rounding mode. The conversion from f64 to f32 is typically done using round to nearest, and the target to round can differ between u64 and u64 to f64. We should round over u64, not u64 to f64.

Initially, I suspected that LLVM might have similar issues as MSVC. The results from the two different implementations were identical, which confused me a bit.

[RKSimon]

llvm/test/CodeGen/X86/uint64-to-float.ll

4

add win64 test coverage as well to check we're not doing this there

[craig.topper]

LGTM

[lebedev.ri]

This change in it's current form does look reasonable to me.
Thanks

[icedrocket]

@craig.topper, could you please commit this change? I don't have permission to commit.

[RKSimon]

In D141074#4061168, @icedrocket wrote:

@craig.topper, could you please commit this change? I don't have permission to commit.

@icedrocket Please can you provide your email address for the commit message?

[icedrocket]

In D141074#4061556, @RKSimon wrote:

In D141074#4061168, @icedrocket wrote:

@craig.topper, could you please commit this change? I don't have permission to commit.

@icedrocket Please can you provide your email address for the commit message?

[e-mail]

[icedrocket]

Could someone commit this change?

[aeubanks]

this is causing us to get undefined symbol errors for ___floatundisf because we don't link in builtins on win32. not 100% sure on this, but I don't think we're expected to link against builtins on win32

https://bugs.chromium.org/p/chromium/issues/detail?id=1408807

[srj]

In D141074#4066869, @aeubanks wrote:

this is causing us to get undefined symbol errors for ___floatundisf because we don't link in builtins on win32. not 100% sure on this, but I don't think we're expected to link against builtins on win32

https://bugs.chromium.org/p/chromium/issues/detail?id=1408807

Same problem was injected into Halide. Are we expected to link in the builtins now? Halide is willing to follow Chrome's lead, but this seems like a surprising change.

[craig.topper]

I'm going to revert this patch. I'm going to see if we can change the precision control around the problematic fadd in the inlined sequence.

llvm/lib/Target/X86/X86ISelLowering.cpp

if (IsStrict)
                          {Chain, Ext});
 
     return DAG.getNode(ISD::SINT_TO_FP, dl, VT, Ext);
   }
 
   if (VT == MVT::f128 || !Subtarget.hasX87())
     return SDValue();
 
+  // On Windows, the default precision control on x87 is only 53-bit.
+  if (SrcVT == MVT::i64 && Subtarget.isOSWindows())
+    return SDValue();
+
   SDValue ValueToStore = Src;
   if (SrcVT == MVT::i64 && Subtarget.hasSSE2() && !Subtarget.is64Bit())
     // Bitcasting to f64 here allows us to do a single 64-bit store from
     // an SSE register, avoiding the store forwarding penalty that would come
     // with two 32-bit stores.
     ValueToStore = DAG.getBitcast(MVT::f64, ValueToStore);
 
   unsigned Size = SrcVT.getStoreSize();
std::pair<SDValue, SDValue> Tmp =
         BuildFILD(DstVT, MVT::i64, dl, Store2, StackSlot, MPI, SlotAlign, DAG);
     if (IsStrict)
       return DAG.getMergeValues({Tmp.first, Tmp.second}, dl);
 
     return Tmp.first;
   }
 
   assert(SrcVT == MVT::i64 && "Unexpected type in UINT_TO_FP");
+
+  // On Windows, the default precision control on x87 is only 53-bit.
+  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
     // an SSE register, avoiding the store forwarding penalty that would come
     // with two 32-bit stores.
     ValueToStore = DAG.getBitcast(MVT::f64, ValueToStore);
   }
   SDValue Store =

llvm/test/CodeGen/X86/uint64-to-float.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-apple-unknown -mattr=+sse2 | FileCheck %s --check-prefix=X86
-; RUN: llc < %s -mtriple=x86_64-apple-unknown -mattr=+sse2 | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=i686-unknown -mattr=+sse2 | FileCheck %s --check-prefix=X86
+; RUN: llc < %s -mtriple=x86_64-unknown -mattr=+sse2 | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=i686-win32 -mattr=+sse2 | FileCheck %s --check-prefix=X86-WIN32
 
 ; Verify that we are using the efficient uitofp --> sitofp lowering illustrated
 ; by the compiler_rt implementation of __floatundisf.
 ; <rdar://problem/8493982>
 
 define float @test(i64 %a) nounwind {
 ; X86-LABEL: test:
 ; X86:       # %bb.0: # %entry

 ; X64-NEXT:  .LBB0_1:
 ; X64-NEXT:    movq %rdi, %rax
 ; X64-NEXT:    shrq %rax
 ; X64-NEXT:    andl $1, %edi
 ; X64-NEXT:    orq %rax, %rdi
 ; X64-NEXT:    cvtsi2ss %rdi, %xmm0
 ; X64-NEXT:    addss %xmm0, %xmm0
 ; X64-NEXT:    retq
+;
+; X86-WIN32-LABEL: test:
+; X86-WIN32:       # %bb.0: # %entry
+; X86-WIN32-NEXT:    pushl %ebp
+; X86-WIN32-NEXT:    movl %esp, %ebp
+; X86-WIN32-NEXT:    andl $-8, %esp
+; X86-WIN32-NEXT:    subl $8, %esp
+; X86-WIN32-NEXT:    pushl 12(%ebp)
+; X86-WIN32-NEXT:    pushl 8(%ebp)
+; X86-WIN32-NEXT:    calll ___floatundisf
+; X86-WIN32-NEXT:    addl $8, %esp
+; X86-WIN32-NEXT:    movl %ebp, %esp
+; X86-WIN32-NEXT:    popl %ebp
+; X86-WIN32-NEXT:    retl
+
 entry:
   %b = uitofp i64 %a to float
   ret float %b
 }