This is an archive of the discontinued LLVM Phabricator instance.

[X86] Preserve FPSW when popping x87 stack
ClosedPublic

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

Details

Summary

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
llvm/lib/Target/X86/X86InsertWait.cpp
65

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

llvm/test/CodeGen/X86/x87-stack-pop.mir
1

Please use update_mir_test_checks.py

craig.topper added inline comments.Nov 6 2021, 8:12 PM
llvm/lib/Target/X86/X86FloatingPoint.cpp
838

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

847

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.
llvm/lib/Target/X86/X86FloatingPoint.cpp
847

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

Thanks!

craig.topper added inline comments.Nov 7 2021, 4:57 PM
llvm/lib/Target/X86/X86FloatingPoint.cpp
885

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

pengfei added inline comments.Nov 7 2021, 5:22 PM
llvm/lib/Target/X86/X86FloatingPoint.cpp
885

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
llvm/lib/Target/X86/X86FloatingPoint.cpp
885

What about non-X87 instructions getting between them?

pengfei added inline comments.Nov 7 2021, 5:54 PM
llvm/lib/Target/X86/X86FloatingPoint.cpp
885

getNextFPInstruction will skip them.

craig.topper added inline comments.Nov 7 2021, 5:55 PM
llvm/lib/Target/X86/X86FloatingPoint.cpp
885

Oh right. Thank you.

This revision was automatically updated to reflect the committed changes.