miri: implement some `llvm.x86.sse.*` intrinsics and add tests #2989

eduardosm · 2023-07-22T10:34:15Z

Implements LLVM intrisics needed to run most SSE functions from core::arch::x86{,_64}.

Also adds miri tests for those functions (mostly copied from core_arch tests).

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.

rustbot · 2023-07-22T10:34:17Z

Error: The feature assign is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

RalfJung

This looks very good, thanks a lot for all the comments. :) I left a bunch of notes, mostly minor.

I can't actually tell if these implementations are correct, I'll just have to trust you on that and wait for bug reports otherwise. ;)

One thing that might be useful is to briefly establish some terminology at the top of sse.rs. In particular "ss" and "ps" seem to be very common and I have no idea what these are for.

src/shims/foreign_items.rs

src/shims/intrinsics/mod.rs

src/shims/x86/sse.rs

tests/pass/intrinsics-x86-sse.rs

src/shims/x86/sse.rs

RalfJung · 2023-08-08T14:40:29Z

@eduardosm so through this PR I became aware of nontemporal_store, which led to rust-lang/rust#114582, and getcsr/setcsr, which led to rust-lang/stdarch#1454... which other hideous monsters are lurking in stdarch?^^

RalfJung · 2023-08-08T14:42:16Z

Please never merge master into your PR, and only rebase when there are conflicts or it is needed to resolve CI failures (which might be the case here due to the operand_deref rename).

eduardosm · 2023-08-08T14:47:46Z

Please never merge master into your PR, and only rebase when there are conflicts or it is needed to resolve CI failures (which might be the case here due to the operand_deref rename).

I was avoiding force-pushing so everything appears as new commits to make it easier to review. I was planning on squashing everything together at the end.

RalfJung · 2023-08-08T14:53:40Z

Yeah, keeping it in new commits is definitely appreciated, even across the rebase.

I guess the merge commits don't hurt if we remember to squash-before-merge.

src/shims/x86/sse.rs

RalfJung · 2023-08-11T12:02:33Z

I've added a commit that moves the entire "multiply by (1+err)" logic into the helper function, makes it much more clear what's going on IMO. I wonder if it wouldn't be easier to just XOR all but the first 11 or 12 bits of the mantissa with random garbage, but I don't know how to do that with apfloat so 🤷 .

LGTM, thanks a lot for enduring the review! Can you please squash?

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`. Also adds miri tests for those functions (mostly copied from core_arch tests).

eduardosm · 2023-08-11T12:16:47Z

Squashed! Thanks for the thorough review :)

RalfJung · 2023-08-11T13:09:19Z

This test failure here is concerning: https://github.com/rust-lang/miri/actions/runs/5832262620/job/15817332780

This failed here. Is it possible that the tests are expecting more precision from RCP/RSQRT than they provide?

eduardosm · 2023-08-11T13:18:48Z

This test failure here is concerning: https://github.com/rust-lang/miri/actions/runs/5832262620/job/15817332780

This failed here. Is it possible that the tests are expecting more precision from RCP/RSQRT than they provide?

Nah, that's because it was calculating sqrt(x) instead of 1/sqrt(x).

miri/src/shims/x86/sse.rs

Lines 531 to 539 in 31ed6d0

    
           FloatUnaryOp::Rsqrt => { 
        
               let op = op.to_scalar().to_u32()?; 
        
               // FIXME using host floats 
        
               let sqrt = Single::from_bits(f32::from_bits(op).sqrt().to_bits().into()); 
        
               // Apply a relative error with a magnitude on the order of 2^-12 to simulate the 
        
               // inaccuracy of RSQRT. 
        
               let res = apply_random_float_error(this, sqrt, -12); 
        
               Ok(Scalar::from_f32(res)) 
        
           }

I fixed it before squashing.

RalfJung · 2023-08-11T13:46:24Z

Nah, that's because it was calculating sqrt(x) instead of 1/sqrt(x).

Oh, I screwed up then. oops Thanks for taking care of it!

RalfJung · 2023-08-11T13:47:24Z

@bors r+

bors · 2023-08-11T13:47:26Z

📌 Commit 16a4a9f has been approved by RalfJung

It is now in the queue for this repository.

bors · 2023-08-11T13:47:32Z

⌛ Testing commit 16a4a9f with merge c6bd71a...

bors · 2023-08-11T14:35:32Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing c6bd71a to master...

Add checked float-to-int helper function As discussed in #2989 (comment)

Add checked float-to-int helper function As discussed in rust-lang/miri#2989 (comment)

Implement some `llvm.x86.sse2.*` intrinsics and add tests Continuation of #2989 with SSE2 intrinsics. Thankfully, a significant amount of SSE2 functions use `simd_*` intrinsics, which are already implemented in Miri.

Implement some `llvm.x86.sse2.*` intrinsics and add tests Continuation of rust-lang/miri#2989 with SSE2 intrinsics. Thankfully, a significant amount of SSE2 functions use `simd_*` intrinsics, which are already implemented in Miri.

RalfJung reviewed Aug 6, 2023

View reviewed changes

RalfJung reviewed Aug 8, 2023

View reviewed changes

src/shims/x86/sse.rs Show resolved Hide resolved