f32::powi on Windows returns different results between 1.44 and 1.45 beta

[iliekturtles]

The issue was noticed because typenum::Pow::powi returns consistent results between rustc versions while f32::powi changes.

Given the following code the "std powi" results differ between stable and beta/nightly. Note that code below will run on https://play.rust-lang.org/ but the issue will not reproduce because the host is not Windows. The playground's output for stable/beta/nightly matches stable on my local machine.

extern crate typenum; // 2.12.0

fn main() {
    let v = 12.046623229980468750000000000000_f32;
    let a = a(v);
    let b = b(v);

    println!("std powi {:.30?}", a);
    println!("typenum  {:.30?}", b);
}

#[inline(never)]
fn a(v: f32) -> f32 {
    v.powi(3)
}

#[inline(never)]
fn b(v: f32) -> f32 {
    typenum::Pow::powi(v, typenum::P3::new())
}

On my Windows 10 machine (and the Travis Win 10 VMs) I get the following results. I'm in the process of bisecting down to a specific nightly right now.

/d/Code/Test/rust/tst (master)$ cargo run; rustc --version --verbose
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s
     Running `target\debug\tst.exe`
std powi 1748.219482421875000000000000000000
typenum  1748.219482421875000000000000000000
rustc 1.44.0 (49cae5576 2020-06-01)
binary: rustc
commit-hash: 49cae55760da0a43428eba73abcb659bb70cf2e4
commit-date: 2020-06-01
host: x86_64-pc-windows-msvc
release: 1.44.0
LLVM version: 9.0
/d/Code/Test/rust/tst (master)$ cargo +beta run; rustc +beta --version --verbose
   Compiling tst v0.1.0 (D:\Code\Test\rust\tst)
    Finished dev [unoptimized + debuginfo] target(s) in 4.95s
     Running `target\debug\tst.exe`
std powi 1748.219604492187500000000000000000
typenum  1748.219482421875000000000000000000
rustc 1.45.0-beta.3359 (b7dc83a3f 2020-06-03)
binary: rustc
commit-hash: b7dc83a3f6ca9746fb3d121761c3605477b77d90
commit-date: 2020-06-03
host: x86_64-pc-windows-msvc
release: 1.45.0-beta.3359
LLVM version: 10.0

1748.2196044921875
1748.219482421875

[iliekturtles]

Issue first appears in nightly-2020-05-22-x86_64-pc-windows-msvc - rustc 1.45.0-nightly (9310e3b 2020-05-21)

Commit comparison with the previous nightly: 0aa6751...9310e3b

I'm guessing 9f12823 (Update LLVM submodule) is the culprit since the f32::powi code just lowers to an LLVM intrisic.

    Finished dev [unoptimized + debuginfo] target(s) in 2.74s
     Running `target\debug\tst.exe`
std powi 1748.219482421875000000000000000000
typenum  1748.219482421875000000000000000000
rustc 1.45.0-nightly (0aa6751c1 2020-05-20)
binary: rustc
commit-hash: 0aa6751c19d3ba80df5b0b02c00bf44e13c97e80
commit-date: 2020-05-20
host: x86_64-pc-windows-msvc
release: 1.45.0-nightly
LLVM version: 9.0

    Finished dev [unoptimized + debuginfo] target(s) in 9.16s
     Running `target\debug\tst.exe`
std powi 1748.219604492187500000000000000000
typenum  1748.219482421875000000000000000000
rustc 1.45.0-nightly (9310e3bd4 2020-05-21)
binary: rustc
commit-hash: 9310e3bd4f425f84fc27878ebf2bda1f30935a63
commit-date: 2020-05-21
host: x86_64-pc-windows-msvc
release: 1.45.0-nightly
LLVM version: 10.0

[ecstatic-morse]

I'm guessing that LLVM 10 started using the MSVC implementation of the powi intrinsic, which uses a higher precision for intermediate results. The result you see is equivalent to the one you would get from casting to and f64, multiplying and then casting back to and f32.

[ollie27]

Looking at the changes in assembly for powi on 1.44.0 we have:

	callq	__powisf2

but on beta that's replaced with:

	cvtsi2ss	%edx, %xmm1
	callq	powf

So LLVM has switched from calling the __powisf2 intrinsic from compiler-rt/compiler-builtins to calling powf. This looks like an intentional change from https://reviews.llvm.org/D69013 but it's not clear to me why that change was made.

[iliekturtles]

Thanks for digging into the details. Sounds like this issue should be closed as working as intended then?

[ollie27]

Sounds like this issue should be closed as working as intended then?

I don't know. I think minor changes to the output of float operations between versions are technically allowed breakage but it would be nice to know why this change was made.

Given that powi seemed to be working fine before, does anyone know why https://reviews.llvm.org/D69013 was done?

[iliekturtles]

I'll leave the issue open for at least a little bit longer to see if there are any other opinions. I was able to resolve my specific case by refactoring internal code to use f32::powi instead of typenum::Pow::powi. My test code was using an explicit f32::powi and comparing the results against the internal code: iliekturtles/uom#193.

[iliekturtles]

Closing since this is an allowed breakage.

[ecstatic-morse]

Reopening. This is on the compiler team meeting agenda for this week.

[spastorino]

Removing I-nominated label given that this was discussed briefly in today's meeting. Adding also relnotes as discussed.

I think we can close this issue now but just in case, @Mark-Simulacrum can you confirm this? and in particular I guess if you will be able to grab the relnotes label of a closed issue or how that process works.

[ollie27]

Did anyone actually figure out why this change was made?

[Mark-Simulacrum]

My impression is no. I don't think we should close this out before it's a compat note in relnotes and we have some sense of why this is happening (beyond "LLVM patch"), if only to hopefully be better able to explain to users why things broke for them.

[eddyb]

Looks like the intention was to avoid calling __powidf2 as it's (sometimes?) not available in MSVC (so this might be for compiling without compiler-rt?), and they assumed the expansion would be compatible. I'm not sure they were aware it might have a different result, or what the deal is with usecases that involve compiler-rt.

[spastorino]

Given that this was added to release notes, I think this should be closed now?.

[spastorino]

Tagging as P-low until we find some answers for @Mark-Simulacrum's questions

[iliekturtles]

@Mark-Simulacrum @spastorino With this documented in the 1.45.0 compatability notes are there still open questions that need action or are we good to close this issue?

[Mark-Simulacrum]

I think we can close. I'm not fully happy about the explanation we have, but it seems good enough for the time being.