Skip to content

[X86] Add pass to insert EMMS/FEMMS instructions to separate MMX and X87 states #41664

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

Closed
RKSimon opened this issue Jun 19, 2019 · 7 comments
Closed
Labels
backend:X86 bugzilla Issues migrated from bugzilla obsolete Issues with old (unsupported) versions of LLVM

Comments

@RKSimon
Copy link
Collaborator

RKSimon commented Jun 19, 2019

Bugzilla Link 42319
Version trunk
OS Windows NT
Depends On #35330
Blocks #40374
CC @topperc,@efriedma-quic,@gnzlbg,@zmodem,@jyknight,@RKSimon,@nico,@rjmccall,@rnk,@rotateright

Extended Description

As discussed on https://reviews.llvm.org/D59744, we currently have no way to automatically separate MMX and x87 instructions - we rely on manual insertion of _mm_empty() (EMMS) intrinsics.

Ideally we need something like the X86VZeroUpper pass which can recognise when MMX/X87 instructions have been used, insert EMMS/FEMMS instructions where appropriate and ensure that MMX/X87 ops don't cross the barrier (see Bug #​35982).

Given the high cost of EMMS, we may want to make this pass opt-in - for example only enable it by default for the i386 ABI change (see Bug #​41029).

@RKSimon
Copy link
Collaborator Author

RKSimon commented Jun 19, 2019

Codegen showing the need to insert _mm_empty() https://godbolt.org/z/zh3R1b

#include <x86intrin.h>

// BAD: mixes MMX + X87 states
float add(__v2si x, float y) {
    return (float)x[0] + y;
}

// GOOD: EMMS separates MMX + X87 states
float add_safe(__v2si x, float y) {
    int i = x[0];
    _mm_empty();
    return (float)i + y;
}
add:
  pushl %eax
  movd %mm0, %eax
  movl %eax, (%esp)
  fildl (%esp) <-- can't use x87 instruction until MMX state is cleared
  fadds 8(%esp)
  popl %eax
  retl

add_safe:
  pushl %eax
  movd %mm0, %eax
  emms
  movl %eax, (%esp)
  fildl (%esp)
  fadds 8(%esp)
  popl

@zmodem
Copy link
Collaborator

zmodem commented Jun 19, 2019

Naive question maybe, but I want to understand this better:

add:
pushl %eax
movd %mm0, %eax
movl %eax, (%esp)
fildl (%esp) <-- can't use x87 instruction until MMX state is cleared

What does "MMX state is cleared" mean? We've moved %mm0 into %eax at this point and won't use it again, so it doesn't matter that we clobber it. I thought the problem was just that the x87 and mmx registers are aliased. Is there more to it that makes this not work?

fadds 8(%esp)
popl %eax
retl

@gnzlbg
Copy link
Mannequin

gnzlbg mannequin commented Jun 19, 2019

@​Simon

Ideally we need something like the X86VZeroUpper pass which can recognise when MMX/X87 instructions have been used, insert EMMS/FEMMS instructions where appropriate and ensure that MMX/X87 ops don't cross the barrier

It would be interesting for Rust to be able to use such a pass to prevents errors related to missing EMMS/FEMMS instructions. When using the x86_mmx type on x86_64 targets with SSE enabled, MMX registers and intrinsics are still used, EMMS/FEMMS still need to be inserted appropriately. It would be helpful if such a pass would also work there, and would only insert EMMS/FEMMS instructions if MMX registers are actually used, such that one only pays for the cost of the EMMS/FEMMS instructions if one decides to explicitly use the MMX intrinsics (which is something that most people don't do when targeting SSE targets).

@zmodem
Copy link
Collaborator

zmodem commented Jun 19, 2019

Naive question maybe, but I want to understand this better:

add:
pushl %eax
movd %mm0, %eax
movl %eax, (%esp)
fildl (%esp) <-- can't use x87 instruction until MMX state is cleared

What does "MMX state is cleared" mean? We've moved %mm0 into %eax at this
point and won't use it again, so it doesn't matter that we clobber it. I
thought the problem was just that the x87 and mmx registers are aliased. Is
there more to it that makes this not work?

(I think I understand now, see https://reviews.llvm.org/D59744#1550326)

@jyknight
Copy link
Member

jyknight commented Jan 9, 2021

I believe that this proposal is not worth doing -- that, instead, the way to go is to start deprecating and removing x86mmx support from llvm, instead.

Starting, first, by flipping the Clang mm* builtins to use SSE2 instead of MMX for llvm/llvm-bugzilla-archive#42320 , and culminating eventually in removing the x86mmx type from LLVM IR entirely.

Which leaves assembly (either inline or out-of-line) as the only way to generate MMX instructions in LLVM. And since such currently-existing assembly doesn't annotate itself in any way to indicate that it clobbers the x87/mmx switch state, there's no way for the compiler to insert emms automatically.

@workingjubilee
Copy link
Contributor

Rust has long-since removed its support for MMX intrinsics. My understanding is that the plan to torch anything but assembly support in LLVM for MMX has taken effect, so I believe this issue and possibly the associated MMX issues can be closed.

@RKSimon
Copy link
Collaborator Author

RKSimon commented Nov 27, 2024

Closing - we're (slowly) removing MMX codegen handling (such as it was) - if you need to use MMX (and generate EMMS/FEMMS instructions) then you should perform this with inline asm.

@RKSimon RKSimon closed this as completed Nov 27, 2024
@RKSimon RKSimon reopened this Nov 27, 2024
@RKSimon RKSimon closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2024
@EugeneZelenko EugeneZelenko added the obsolete Issues with old (unsupported) versions of LLVM label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 bugzilla Issues migrated from bugzilla obsolete Issues with old (unsupported) versions of LLVM
Projects
None yet
Development

No branches or pull requests

5 participants