[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

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]

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

885

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

[pengfei]

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]

llvm/lib/Target/X86/X86FloatingPoint.cpp

885

What about non-X87 instructions getting between them?

[pengfei]

llvm/lib/Target/X86/X86FloatingPoint.cpp

885

getNextFPInstruction will skip them.

[craig.topper]

llvm/lib/Target/X86/X86FloatingPoint.cpp

885

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) {
+  for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+    const MachineOperand &MO = MI.getOperand(i);
+    if (MO.isReg() && MO.getReg() == X86::FPSW && MO.isDef() && !MO.isDead())
+      return true;
+  }
+  return false;
+}
+
+static bool doesInstructionUseFPSW(MachineInstr &MI) {
+  for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+    const MachineOperand &MO = MI.getOperand(i);
+    if (MO.isReg() && MO.getReg() == X86::FPSW && MO.isUse())
+      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() && doesInstructionUseFPSW(*Next))
+        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

 
 /// 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) {
+bool X86::isX87Instruction(MachineInstr &MI) {
   for (const MachineOperand &MO : MI.operands()) {
     if (!MO.isReg())
       continue;
     if (isX87Reg(MO.getReg()))
       return true;
   }
   return false;
 }
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/test/CodeGen/X86/x87-stack-pop.mir

+# RUN: llc -mtriple=i686-- -run-pass x86-codegen -O2 -o - %s | FileCheck %s
+
+---
+name: func1
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, offset: 0, size: 10, alignment: 16 }
+body:             |
+  bb.0.entry:
+    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
+    renamable $eax = AND32ri killed renamable $eax, 18176, implicit-def dead $eflags
+    renamable $ecx = MOV32r0 implicit-def dead $eflags
+    CMP32ri killed renamable $eax, 1280, implicit-def $eflags
+    renamable $cl = SETCCr 4, implicit killed $eflags, implicit killed $ecx, implicit-def $ecx
+    $eax = COPY killed renamable $ecx
+    RET 0, $eax
+  ; CHECK-LABEL: name: func1
+  ; CHECK:       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
+
+
+...
+---
+name: func2
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, offset: 0, size: 10, alignment: 16 }
+body:             |
+  bb.0.entry:
+    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
+    renamable $eax = AND32ri killed renamable $eax, 18176, implicit-def dead $eflags
+    renamable $ecx = MOV32r0 implicit-def dead $eflags
+    CMP32ri killed renamable $eax, 1280, implicit-def $eflags
+    renamable $cl = SETCCr 4, implicit killed $eflags, implicit killed $ecx, implicit-def $ecx
+    $eax = COPY killed renamable $ecx
+    RET 0, $eax
+  ; CHECK-LABEL: name: func2
+  ; CHECK:       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
+
+...