rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.39k stars 1.54k forks source link

Lint request: deny_mmx_target_feature #3722

Open gnzlbg opened 5 years ago

gnzlbg commented 5 years ago

The Intel compiler has an extremely useful warning (https://software.intel.com/en-us/articles/cdiag964) that triggers if MMX code interfaces with x87 code without an EMMS instruction before the x87 code.

Example (playground):

pub fn init(_x: i32);
fn main() {
    unsafe {
        let m0 = _mm_cvtsi32_si64(1);
        let m1 = _mm_cvtsi32_si64(2);
        let mut m2 = _mm_cvtsi32_si64(4);

        m2 = _mm_add_pi8(m0, m1);
        let value = _mm_cvtsi64_si32(m2);

        // Without a call to _mm_empty()
        // this program has UB
        //_mm_empty();
        init(value);
    }
}

Without the call to _mm_empty no emms instruction is emitted, and the program has undefined behavior.


It would be nice if we could add a lint that errors if:

This lint could be improved in the future by suggesting users where exactly calls to _mm_empty() need to be inserted, e.g.:

init(value);
^^^^^^^^^^ error: no EMMS instruction before call

note: Use the EMMS instruction (e.g.  by calling the _mm_empty() intrinsic ) 
after the MMX instructions immediately before the x87 code to restore 
the Floating-point status on the CPU.
oli-obk commented 5 years ago

So basically calling any function that does not start with _mm_ after a function that does start with _mm_ (but is not _mm_empty)?

gnzlbg commented 5 years ago

If you use an MMX register, you have to execute the EMMS instruction before using the x87 FPU, otherwise you don't have to execute EMMS.

In Rust, the only way to use an MMX register is to use a type that matches #[repr(simd)] struct _(i64);, like core::arch::x86_64::__m64 (see https://github.com/rust-lang/rust/issues/57832).

So AFAIK, one has to call EMMS before using an f32 or f64 type (cc @rkruppe). It doesn't really matter whether that happens inside the same function as the MMX register is used, or in a different function. Functions that follow the SysV AMD64 ABI assume that this has happened, but target-features can influence a function ABI.


So basically calling any function that does not start with mm after a function that does start with mm (but is not _mm_empty)?

I don't know if this is a good heuristic. Some _mm_ functions might be called following the SysV AMD64 ABI (e.g. not all intrinsics are MMX intrinsics, and some are "composites"), so not warning on these would lead to UB.

A better heuristic might be:

Let MMX_INTRIN be the set of functions in {core, std}::arch::{x86,x86_64} with #[target_feature(enable = "mmx")] or taking or returning the type {core, std}::arch::{x86,x86_64}::__m64.

If an MMX_INTRIN function is called, and

then the warning should be triggered if right before that function call _mm_empty() is not called.

gnzlbg commented 5 years ago

A simpler lint might be to just warn when MMX intrinsics are used as I originally proposed. That is, if the user calls a function with #[target_feature(enable = "mmx")] or defines a function with #[target_feature(enable = "mmx")], then we should just emit a warning.

hanna-kruppe commented 5 years ago

So AFAIK, one has to call EMMS before using an f32 or f64 type

Only when SSE (i.e., the XMM registers) is not available AFAIK. If it's available, all floating point work ought to happen in XMM registers rather than on the x87 stack.

gnzlbg commented 5 years ago

Only when SSE (i.e., the XMM registers) is not available AFAIK. If it's available, all floating point work ought to happen in XMM registers rather than on the x87 stack.

You are referring here exclusively to when floating-point is used with the same function as the MMX registers right?

hanna-kruppe commented 5 years ago

I'm not sure I follow. I just mean that when you use f32 and f64 in a function where SSE is available, the code generated due to the use of f32/f64 doesn't affect MMX registers AKA the x87 (edit: the FPU stack) stack at all. I don't know off-hand how that interacts with the ABI issues surrounding this.

petrochenkov commented 5 years ago

The Intel compiler has an extremely useful warning (https://software.intel.com/en-us/articles/cdiag964) that triggers if MMX code interfaces with x87 code without an EMMS instruction before the x87 code.

I'm not sure it's still extremely useful in 2019, rather than in 1999.

gnzlbg commented 5 years ago

@rkruppe

I don't know off-hand how that interacts with the ABI issues surrounding this.

The SysV AMD64 ABI states:

The CPU shall be in x87 mode upon entry to a function. Therefore, every function that uses the MMX registers is required to issue an emms or femms instruction after using MMX registers, before returning or calling another function.

gnzlbg commented 5 years ago

@petrochenkov

I'm not sure it's still extremely useful in 2019, rather than in 1999.

Fair point. Its useful for code that uses MMX intrinsics directly, but you have a point that probably no code should be doing that anymore. A lint that warns on usage of #[target_feature(enable = "mmx")] and on usage of functions that have that attribute would discourage doing that, while at the same time alerting of its perils.

hanna-kruppe commented 5 years ago

Wild idea, but how about just deprecating all mmx intrinsics directly?

gnzlbg commented 5 years ago

@rkruppe FYI they haven't been stabilized yet.

hanna-kruppe commented 5 years ago

Good to know. But I assume someone somewhere will want these intrinsics for porting legacy code from C, leading to their eventual stabilization. At the same time, since MMX is completely irrelevant for new software and has dangerous pitfalls, it seems a good candidate for deprecation in the sense of "this is obsolete and dangerous so don't use it in new code".

gnzlbg commented 5 years ago

That's a fair option.

workingjubilee commented 4 years ago

Rust no longer supports using MMX intrinsics or emitting MMX instructions (except possibly via inline assembly). This issue can be closed.