Skip to content
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

Implement {float}::asin_acos(), {float}::asin(), and {float}::acos() #48

Merged
merged 5 commits into from
Aug 28, 2020

Conversation

fu5ha
Copy link
Contributor

@fu5ha fu5ha commented Aug 21, 2020

Fixes #44

@fu5ha
Copy link
Contributor Author

fu5ha commented Aug 21, 2020

Due to the tests for this PR hitting an obscure bug relating to f32::from_bits() quieting a "signalling nan" causing an off-by-one-bit (that could have happened at any other time elsewhere technically i guess), it depends on #43 to be in first in order to pass tests on i586 hardware (non-simd codepath)

@MaulingMonkey
Copy link

MaulingMonkey commented Aug 21, 2020

Huzzah for test coverage!

Cross referencing rust-lang/rust#73328 for upstream information about x not necessairly round-tripping through f32::from_bits(x).to_bits(). I'm not entirely sure if it's considered a codegen bug, or if f32::from_bits just needs better documentation, but considering how deep into the bowels of LLVM behavior you'd need to go to fix this, I wouldn't hold my breath about Rust's stdlib behavior getting changed anytime soon.

A concrete example for posterity:

fn main() {
    let bits : u32 = 0x7f8d9fa6; // some negative signaling (x86/ARM) NaN
    let f = f32::from_bits(bits);
    assert_eq!(bits, f.to_bits()); // will fail, now equal to 0x7fcd9fa6
}

And some annotated disassembly of f32::from_bits:

    #[stable(feature = "float_bits_conv", since = "1.20.0")]
    #[inline]
    pub fn from_bits(v: u32) -> Self {
007F3300  sub         esp,8  
007F3303  mov         eax,dword ptr [esp+0Ch]                                   EAX := 7F8D9FA6
        // SAFETY: `u32` is a plain old datatype so we can always transmute from it
        // It turns out the safety issues with sNaN were overblown! Hooray!
        unsafe { mem::transmute(v) }
007F3307  mov         dword ptr [esp+4],eax                                     *(unsigned int*)(ESP+4),x := 0x7f8d9fa6     Expected
007F330B  fld         dword ptr [esp+4]                                         ST0 = 1#QNAN 
007F330F  fstp        dword ptr [esp]                                           *(unsigned int*)(ESP+0),x := 0x7fcd9fa6     High bit of the mantissa set
    }
007F3312  fld         dword ptr [esp]  
007F3315  add         esp,8  
007F3318  ret  

(EDIT: And a discussion thread for posterity too: https://discord.com/channels/273534239310479360/335502453371961344/746445211260944444)

@fu5ha
Copy link
Contributor Author

fu5ha commented Aug 28, 2020

@Lokathor i rebased this on the avx changes and added impls for those types as well, and I also #[cfg]'d out the tests that don't pass for now with FIXME's to put them back in once #43 lands

@Lokathor
Copy link
Owner

whoops forgot about this.

we good here?

@fu5ha
Copy link
Contributor Author

fu5ha commented Aug 28, 2020 via email

@Lokathor Lokathor merged commit 55b870c into Lokathor:main Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{floating}::acos()
3 participants