This is an archive of the discontinued LLVM Phabricator instance.

[X86] Preserve FPSW when popping x87 stack

Authored by sepavloff on Nov 6 2021, 1:19 AM.



When compiler converts x87 operations to stack model, it may insert
instructions that pop top stack element. To do it the compiler inserts
instruction FSTP right after the instruction that calculates value on
the stack. It can break the code that uses FPSW set by the last
instruction. For example, an instruction FXAM is usually followed by
FNSTSW, but FSTP is inserted after FXAM. As FSTP leaves condition code
in FPSW undefined, the compiler produces incorrect code.

With this change FSTP in inserted after the FPSW consumer if the last
instruction sets FPSW.

Diff Detail

Event Timeline

sepavloff created this revision.Nov 6 2021, 1:19 AM
sepavloff requested review of this revision.Nov 6 2021, 1:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2021, 1:19 AM
craig.topper added inline comments.Nov 6 2021, 8:07 PM

This function should move to X86InstrInfo.cpp if it is going to be in the X86InstrInfo.h header.


Please use

craig.topper added inline comments.Nov 6 2021, 8:12 PM

Can this use MachineInstr::findRegisterDefOperand? You'll have to check the dead isDead flag after.


Can this use MachineInstr::readsRegister

sepavloff updated this revision to Diff 385339.Nov 7 2021, 6:30 AM

Update patch

  • function doesInstructionSetFPSW is implemented using MachineInstr::findRegisterDefOperand,
  • function doesInstructionUseFPSW is removed, MachineInstr::readsRegister is used instead,
  • function isX87Instruction is moved to X86InstrInfo.cpp,
  • tests are simplified,
  • a test for dead definition is added.
sepavloff marked 4 inline comments as done.Nov 7 2021, 6:32 AM
sepavloff added inline comments.

Used MachineInstr::readsRegister directly.

pengfei accepted this revision.Nov 7 2021, 7:04 AM

LGTM. Please wait one or two days for others' comments.

This revision is now accepted and ready to land.Nov 7 2021, 7:04 AM
sepavloff marked an inline comment as done.Nov 7 2021, 7:37 AM


craig.topper added inline comments.Nov 7 2021, 4:57 PM

Is checking the next instruction sufficient? Can other instructions be scheduled before the FNSTSW?

pengfei added inline comments.Nov 7 2021, 5:22 PM

I think it's sufficient. On one hand, most X87 instructions will def FPSW. So they won't have any chance to be scheduled before a use of FPSW. On the other hand, even if one X87 instruction neither def FPSW nor use it, it should always touch ST registers. Poping ST0 after the use of ST registers doesn't seem correct to me.

craig.topper added inline comments.Nov 7 2021, 5:34 PM

What about non-X87 instructions getting between them?

pengfei added inline comments.Nov 7 2021, 5:54 PM

getNextFPInstruction will skip them.

craig.topper added inline comments.Nov 7 2021, 5:55 PM

Oh right. Thank you.

This revision was automatically updated to reflect the committed changes.