[X86] Preserve FPSW when popping x87 stack

[sepavloff]

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.

[craig.topper]

llvm/lib/Target/X86/X86InsertWait.cpp

57–58

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

2

Please use update_mir_test_checks.py

[craig.topper]

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]

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]

llvm/lib/Target/X86/X86FloatingPoint.cpp

847

Used MachineInstr::readsRegister directly.

[pengfei]

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

[sepavloff]

Thanks!

[craig.topper]

llvm/lib/Target/X86/X86FloatingPoint.cpp

874

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

[pengfei]

llvm/lib/Target/X86/X86FloatingPoint.cpp

874

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]

llvm/lib/Target/X86/X86FloatingPoint.cpp

874

What about non-X87 instructions getting between them?

[pengfei]

llvm/lib/Target/X86/X86FloatingPoint.cpp

874

getNextFPInstruction will skip them.

[craig.topper]

llvm/lib/Target/X86/X86FloatingPoint.cpp

874

Oh right. Thank you.

llvm/lib/Target/X86/X86FloatingPoint.cpp

static const TableEntry PopTable[] = {
   { X86::SUB_FrST0 , X86::SUB_FPrST0  },
 
   { X86::UCOM_FIr  , X86::UCOM_FIPr   },
 
   { X86::UCOM_FPr  , X86::UCOM_FPPr   },
   { X86::UCOM_Fr   , X86::UCOM_FPr    },
 };
 
+static bool doesInstructionSetFPSW(MachineInstr &MI) {
+  if (const MachineOperand *MO = MI.findRegisterDefOperand(X86::FPSW))
+    if (!MO->isDead())
+      return true;
+  return false;
+}
+
+static MachineBasicBlock::iterator
+getNextFPInstruction(MachineBasicBlock::iterator I) {
+  MachineBasicBlock &MBB = *I->getParent();
+  while (++I != MBB.end()) {
+    MachineInstr &MI = *I;
+    if (X86::isX87Instruction(MI))
+      return I;
+  }
+  return MBB.end();
+}
+
 /// popStackAfter - Pop the current value off of the top of the FP stack after
 /// the specified instruction.  This attempts to be sneaky and combine the pop
 /// into the instruction itself if possible.  The iterator is left pointing to
 /// the last instruction, be it a new pop instruction inserted, or the old
 /// instruction if it was modified in place.
 ///
 void FPS::popStackAfter(MachineBasicBlock::iterator &I) {
   MachineInstr &MI = *I;
   const DebugLoc &dl = MI.getDebugLoc();
   ASSERT_SORTED(PopTable);
 
   popReg();
 
   // Check to see if there is a popping version of this instruction...
   int Opcode = Lookup(PopTable, I->getOpcode());
   if (Opcode != -1) {
     I->setDesc(TII->get(Opcode));
     if (Opcode == X86::FCOMPP || Opcode == X86::UCOM_FPPr)
       I->RemoveOperand(0);
     MI.dropDebugNumber();
   } else {    // Insert an explicit pop
+    // If this instruction sets FPSW, which is read in following instruction,
+    // insert pop after that reader.
+    if (doesInstructionSetFPSW(MI)) {
+      MachineBasicBlock &MBB = *MI.getParent();
+      MachineBasicBlock::iterator Next = getNextFPInstruction(I);
+      if (Next != MBB.end() && Next->readsRegister(X86::FPSW))
+        I = Next;
+    }
     I = BuildMI(*MBB, ++I, dl, TII->get(X86::ST_FPrr)).addReg(X86::ST0);
   }
 }
 
 /// freeStackSlotAfter - Free the specified register from the register stack, so
 /// that it is no longer in a register.  If the register is currently at the top
 /// of the stack, we just pop the current instruction, otherwise we store the
 /// current top-of-stack into the specified slot, then pop the top of stack.

llvm/lib/Target/X86/X86InsertWait.cpp

public:
   }
 };
 
 } // namespace
 
 char WaitInsert::ID = 0;
 
 FunctionPass *llvm::createX86InsertX87waitPass() { return new WaitInsert(); }
 
-/// Return true if the Reg is X87 register.
-static bool isX87Reg(unsigned Reg) {
-  return (Reg == X86::FPCW || Reg == X86::FPSW ||
-          (Reg >= X86::ST0 && Reg <= X86::ST7));
-}
-
-/// check if the instruction is X87 instruction
-static bool isX87Instruction(MachineInstr &MI) {
-  for (const MachineOperand &MO : MI.operands()) {
-    if (!MO.isReg())
-      continue;
-    if (isX87Reg(MO.getReg()))
-      return true;
-  }
-  return false;
-}
-
 static bool isX87ControlInstruction(MachineInstr &MI) {
   switch (MI.getOpcode()) {
   case X86::FNINIT:
   case X86::FLDCW16m:
   case X86::FNSTCW16m:
   case X86::FNSTSW16r:
   case X86::FNSTSWm:
   case X86::FNCLEX:
   case X86::FLDENVm:
bool WaitInsert::runOnMachineFunction(MachineFunction &MF) {
 
   const X86Subtarget &ST = MF.getSubtarget<X86Subtarget>();
   const X86InstrInfo *TII = ST.getInstrInfo();
   bool Changed = false;
 
   for (MachineBasicBlock &MBB : MF) {
     for (MachineBasicBlock::iterator MI = MBB.begin(); MI != MBB.end(); ++MI) {
       // Jump non X87 instruction.
-      if (!isX87Instruction(*MI))
+      if (!X86::isX87Instruction(*MI))
         continue;
       // If the instruction instruction neither has float exception nor is
       // a load/store instruction, or the instruction is x87 control
       // instruction, do not insert wait.
       if (!(MI->mayRaiseFPException() || MI->mayLoadOrStore()) ||
           isX87ControlInstruction(*MI))
         continue;
       // If the following instruction is an X87 instruction and isn't an X87
       // non-waiting control instruction, we can omit insert wait instruction.
       MachineBasicBlock::iterator AfterMI = std::next(MI);
-      if (AfterMI != MBB.end() && isX87Instruction(*AfterMI) &&
+      if (AfterMI != MBB.end() && X86::isX87Instruction(*AfterMI) &&
           !isX87NonWaitingControlInstruction(*AfterMI))
         continue;
 
       BuildMI(MBB, AfterMI, MI->getDebugLoc(), TII->get(X86::WAIT));
       LLVM_DEBUG(dbgs() << "\nInsert wait after:\t" << *MI);
       // Jump the newly inserting wait
       ++MI;
       Changed = true;
     }
   }
   return Changed;
 }

llvm/lib/Target/X86/X86InstrFPStack.td

 def TST_Fp64  : FpIf64<(outs), (ins RFP64:$src), OneArgFP, []>;
 def TST_Fp80  : FpI_<(outs), (ins RFP80:$src), OneArgFP, []>;
 } // hasSideEffects
 
 def TST_F  : FPI<0xD9, MRM_E4, (outs), (ins), "ftst">;
 } // SchedRW
 } // Uses = [FPCW], mayRaiseFPException = 1
 
-let SchedRW = [WriteFTest] in {
+let SchedRW = [WriteFTest], Defs = [FPSW] in {
 def XAM_Fp32  : FpIf32<(outs), (ins RFP32:$src), OneArgFP, []>;
 def XAM_Fp64  : FpIf64<(outs), (ins RFP64:$src), OneArgFP, []>;
 def XAM_Fp80  : FpI_<(outs), (ins RFP80:$src), OneArgFP, []>;
 def XAM_F     : FPI<0xD9, MRM_E5, (outs), (ins), "fxam">;
 } // SchedRW
 
 // Versions of FP instructions that take a single memory operand.  Added for the
 //   disassembler; remove as they are included with patterns elsewhere.

llvm/lib/Target/X86/X86InstrInfo.h

 unsigned getSwappedVPCMPImm(unsigned Imm);
 
 /// Get the VPCOM immediate if the opcodes are swapped.
 unsigned getSwappedVPCOMImm(unsigned Imm);
 
 /// Get the VCMP immediate if the opcodes are swapped.
 unsigned getSwappedVCMPImm(unsigned Imm);
 
+/// Check if the instruction is X87 instruction.
+bool isX87Instruction(MachineInstr &MI);
 } // namespace X86
 
 /// isGlobalStubReference - Return true if the specified TargetFlag operand is
 /// a reference to a stub for a global, not the global itself.
 inline static bool isGlobalStubReference(unsigned char TargetFlag) {
   switch (TargetFlag) {
   case X86II::MO_DLLIMPORT:               // dllimport stub.
   case X86II::MO_GOTPCREL:                // rip-relative GOT reference.

llvm/lib/Target/X86/X86InstrInfo.cpp

case 0x01: case 0x02:
     // Need to toggle bits 3:0. Bit 4 stays the same.
     Imm ^= 0xf;
     break;
   }
 
   return Imm;
 }
 
+/// Return true if the Reg is X87 register.
+static bool isX87Reg(unsigned Reg) {
+  return (Reg == X86::FPCW || Reg == X86::FPSW ||
+          (Reg >= X86::ST0 && Reg <= X86::ST7));
+}
+
+/// check if the instruction is X87 instruction
+bool X86::isX87Instruction(MachineInstr &MI) {
+  for (const MachineOperand &MO : MI.operands()) {
+    if (!MO.isReg())
+      continue;
+    if (isX87Reg(MO.getReg()))
+      return true;
+  }
+  return false;
+}
+
 bool X86InstrInfo::isUnconditionalTailCall(const MachineInstr &MI) const {
   switch (MI.getOpcode()) {
   case X86::TCRETURNdi:
   case X86::TCRETURNri:
   case X86::TCRETURNmi:
   case X86::TCRETURNdi64:
   case X86::TCRETURNri64:
   case X86::TCRETURNmi64:

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

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=i686-- -run-pass x86-codegen -O2 -o - %s | FileCheck %s
+
+---
+name: func_fxam
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, offset: 0, size: 10, alignment: 16 }
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: func_fxam
+    ; CHECK: nofpexcept LD_F80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw, implicit-def $st0 :: (load (s80) from %fixed-stack.0, align 16)
+    ; CHECK-NEXT: XAM_F implicit-def $fpsw, implicit $st0
+    ; CHECK-NEXT: FNSTSW16r implicit-def $ax, implicit-def dead $fpsw, implicit $fpsw
+    ; CHECK-NEXT: ST_FPrr $st0, implicit-def $fpsw, implicit $fpcw
+    ; CHECK-NEXT: renamable $ax = KILL $ax, implicit-def $eax
+    ; CHECK-NEXT: RET 0, $eax
+    renamable $fp0 = nofpexcept LD_Fp80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw :: (load (s80) from %fixed-stack.0, align 16)
+    XAM_Fp80 killed renamable $fp0, implicit-def $fpsw
+    FNSTSW16r implicit-def $ax, implicit-def dead $fpsw, implicit $fpsw
+    renamable $ax = KILL $ax, implicit-def $eax
+    RET 0, $eax
+
+
+...
+---
+name: func_ftst
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, offset: 0, size: 10, alignment: 16 }
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: func_ftst
+    ; CHECK: nofpexcept LD_F80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw, implicit-def $st0 :: (load (s80) from %fixed-stack.0, align 16)
+    ; CHECK-NEXT: TST_F implicit-def $fpsw, implicit $fpcw, implicit $st0
+    ; CHECK-NEXT: FNSTSW16r implicit-def $ax, implicit-def dead $fpsw, implicit $fpsw
+    ; CHECK-NEXT: ST_FPrr $st0, implicit-def $fpsw, implicit $fpcw
+    ; CHECK-NEXT: renamable $ax = KILL $ax, implicit-def $eax
+    ; CHECK-NEXT: RET 0, $eax
+    renamable $fp0 = nofpexcept LD_Fp80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw :: (load (s80) from %fixed-stack.0, align 16)
+    TST_Fp80 killed renamable $fp0, implicit-def $fpsw, implicit $fpcw
+    FNSTSW16r implicit-def $ax, implicit-def dead $fpsw, implicit $fpsw
+    renamable $ax = KILL $ax, implicit-def $eax
+    RET 0, $eax
+
+...
+---
+name: func_deaddef
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, offset: 0, size: 10, alignment: 16 }
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: name: func_deaddef
+    ; CHECK: nofpexcept LD_F80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw, implicit-def $st0 :: (load (s80) from %fixed-stack.0, align 16)
+    ; CHECK-NEXT: TST_F implicit-def dead $fpsw, implicit $fpcw, implicit $st0
+    ; CHECK-NEXT: ST_FPrr $st0, implicit-def $fpsw, implicit $fpcw
+    ; CHECK-NEXT: FNSTSW16r implicit-def $ax, implicit-def dead $fpsw, implicit $fpsw
+    ; CHECK-NEXT: renamable $ax = KILL $ax, implicit-def $eax
+    ; CHECK-NEXT: RET 0, $eax
+    renamable $fp0 = nofpexcept LD_Fp80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw :: (load (s80) from %fixed-stack.0, align 16)
+    TST_Fp80 killed renamable $fp0, implicit-def dead $fpsw, implicit $fpcw
+    FNSTSW16r implicit-def $ax, implicit-def dead $fpsw, implicit $fpsw
+    renamable $ax = KILL $ax, implicit-def $eax
+    RET 0, $eax
+
+...