LGTM

I wrote some code to verify that u64 to f64 conversion using FILD+FADD with 53-bit precision is accurate. I tested it with 10^12 cases and the results are: u64 to f32 conversion failed frequently, but u64 to f64 conversion does not failed.

In D142178#4100370, @craig.topper wrote:

Remove unrelated comment change. The line is longer than 80 columns, but isn't near code this is touching.

Could you update the diff? The current diff is outdated and cannot be applied to main branch automatically.

I tested the patch and it seems to have fixed the issue.

Could someone commit this change?

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?

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

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.

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.

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 tests keep failing, but it's all RISC-V related, so the diff probably isn't wrong.

You're right, I should have checked enough before request a review.

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.

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

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

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

Fix the comments

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?

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.

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.

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.

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).

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.

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

What to do if user changes the default precision?

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.