Fix PR26410: failing assertion when the X87 stackifier runs on functions containing IRETs

[DavidKreitzer]

Summary

handleSpecialFP is currently hitting llvm_unreachable("Unknown SpecialFP instruction!"), because the IRET opcodes are marked as "FPForm = SpecialFP" but are unsupported in handleSpecialFP.

I chose to fix this by handling all the return opcodes uniformly in handleSpecialFP, as that seemed like the most robust choice. An alternative would be to change X86InstrControl.td so that the IRETs are not marked "FPForm = SpecialFP" since interrupt handlers cannot actually return x87 values (or any other values for that matter). But that clutters X86InstrControl.td for no good reason.

The code in the new handleReturn function is virtually identical to the original return-handling code in handleSpecialFP. The only differences are in the indentation and the "MachineInstr *MI = I;" at the top.

[aaboud]

LGTM.
By the way it should solve this Bug:
https://llvm.org/bugs/show_bug.cgi?id=26410

[DavidKreitzer]

Thanks, Amjad. I took ownership of 26410.

[DavidKreitzer]

Ping.

Amjad recommended that I get a second reviewer/approver before committing. I'd appreciate it if one of you could take a look.

[qcolombet]

Hi David,

The change LGTM, but the tests need a few nitpicks.

Cheers,
-Quentin

test/CodeGen/X86/x86-32-intrcc.ll

83

Could you check we generate sensible code here?

86

Please give a name to %[0-9]+ variables.
opt -instnamer can do this for you.
(It is a pain to update test cases with numbered variables.)

test/CodeGen/X86/x86-64-intrcc.ll

90

Ditto: no %[0-9]+ and sensible testing.

[DavidKreitzer]

Thanks for the review, Quentin. I updated the tests according to your comments.

[qcolombet]

Hi David,

LGTM.

Thanks,
-Quentin

[DavidKreitzer]

Rebased to latest trunk.

lib/Target/X86/X86FloatingPoint.cpp

#endif
     /// Shuffle the top FixCount stack entries such that FP reg FixStack[0] is
     /// st(0), FP reg FixStack[1] is st(1) etc.
     void shuffleStackTop(const unsigned char *FixStack, unsigned FixCount,
                          MachineBasicBlock::iterator I);
 
     bool processBasicBlock(MachineFunction &MF, MachineBasicBlock &MBB);
 
     void handleCall(MachineBasicBlock::iterator &I);
+    void handleReturn(MachineBasicBlock::iterator &I);
     void handleZeroArgFP(MachineBasicBlock::iterator &I);
     void handleOneArgFP(MachineBasicBlock::iterator &I);
     void handleOneArgFPRW(MachineBasicBlock::iterator &I);
     void handleTwoArgFP(MachineBasicBlock::iterator &I);
     void handleCompareFP(MachineBasicBlock::iterator &I);
     void handleCondMovFP(MachineBasicBlock::iterator &I);
     void handleSpecialFP(MachineBasicBlock::iterator &I);
 
void FPS::handleCall(MachineBasicBlock::iterator &I) {
   // FP registers used for function return must be consecutive starting at
   // FP0.
   assert(STReturns == 0 || (isMask_32(STReturns) && N <= 2));
 
   for (unsigned I = 0; I < N; ++I)
     pushReg(N - I - 1);
 }
 
+/// If RET has an FP register use operand, pass the first one in ST(0) and
+/// the second one in ST(1).
+void FPS::handleReturn(MachineBasicBlock::iterator &I) {
+  MachineInstr *MI = I;
+
+  // Find the register operands.
+  unsigned FirstFPRegOp = ~0U, SecondFPRegOp = ~0U;
+  unsigned LiveMask = 0;
+
+  for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
+    MachineOperand &Op = MI->getOperand(i);
+    if (!Op.isReg() || Op.getReg() < X86::FP0 || Op.getReg() > X86::FP6)
+      continue;
+    // FP Register uses must be kills unless there are two uses of the same
+    // register, in which case only one will be a kill.
+    assert(Op.isUse() &&
+           (Op.isKill() ||                        // Marked kill.
+            getFPReg(Op) == FirstFPRegOp ||       // Second instance.
+            MI->killsRegister(Op.getReg())) &&    // Later use is marked kill.
+           "Ret only defs operands, and values aren't live beyond it");
+
+    if (FirstFPRegOp == ~0U)
+      FirstFPRegOp = getFPReg(Op);
+    else {
+      assert(SecondFPRegOp == ~0U && "More than two fp operands!");
+      SecondFPRegOp = getFPReg(Op);
+    }
+    LiveMask |= (1 << getFPReg(Op));
+
+    // Remove the operand so that later passes don't see it.
+    MI->RemoveOperand(i);
+    --i;
+    --e;
+  }
+
+  // We may have been carrying spurious live-ins, so make sure only the
+  // returned registers are left live.
+  adjustLiveRegs(LiveMask, MI);
+  if (!LiveMask) return;  // Quick check to see if any are possible.
+
+  // There are only four possibilities here:
+  // 1) we are returning a single FP value.  In this case, it has to be in
+  //    ST(0) already, so just declare success by removing the value from the
+  //    FP Stack.
+  if (SecondFPRegOp == ~0U) {
+    // Assert that the top of stack contains the right FP register.
+    assert(StackTop == 1 && FirstFPRegOp == getStackEntry(0) &&
+           "Top of stack not the right register for RET!");
+
+    // Ok, everything is good, mark the value as not being on the stack
+    // anymore so that our assertion about the stack being empty at end of
+    // block doesn't fire.
+    StackTop = 0;
+    return;
+  }
+
+  // Otherwise, we are returning two values:
+  // 2) If returning the same value for both, we only have one thing in the FP
+  //    stack.  Consider:  RET FP1, FP1
+  if (StackTop == 1) {
+    assert(FirstFPRegOp == SecondFPRegOp && FirstFPRegOp == getStackEntry(0)&&
+           "Stack misconfiguration for RET!");
+
+    // Duplicate the TOS so that we return it twice.  Just pick some other FPx
+    // register to hold it.
+    unsigned NewReg = ScratchFPReg;
+    duplicateToTop(FirstFPRegOp, NewReg, MI);
+    FirstFPRegOp = NewReg;
+  }
+
+  /// Okay we know we have two different FPx operands now:
+  assert(StackTop == 2 && "Must have two values live!");
+
+  /// 3) If SecondFPRegOp is currently in ST(0) and FirstFPRegOp is currently
+  ///    in ST(1).  In this case, emit an fxch.
+  if (getStackEntry(0) == SecondFPRegOp) {
+    assert(getStackEntry(1) == FirstFPRegOp && "Unknown regs live");
+    moveToTop(FirstFPRegOp, MI);
+  }
+
+  /// 4) Finally, FirstFPRegOp must be in ST(0) and SecondFPRegOp must be in
+  /// ST(1).  Just remove both from our understanding of the stack and return.
+  assert(getStackEntry(0) == FirstFPRegOp && "Unknown regs live");
+  assert(getStackEntry(1) == SecondFPRegOp && "Unknown regs live");
+  StackTop = 0;
+}
+
 /// handleZeroArgFP - ST(0) = fld0    ST(0) = flds <mem>
 ///
 void FPS::handleZeroArgFP(MachineBasicBlock::iterator &I) {
   MachineInstr *MI = I;
   unsigned DestReg = getFPReg(MI->getOperand(0));
 
   // Change from the pseudo instruction to the concrete instruction.
   MI->RemoveOperand(0);   // Remove the explicit ST(0) operand

 void FPS::handleSpecialFP(MachineBasicBlock::iterator &Inst) {
   MachineInstr *MI = Inst;
 
   if (MI->isCall()) {
     handleCall(Inst);
     return;
   }
 
+  if (MI->isReturn()) {
+    handleReturn(Inst);
+    return;
+  }
+
   switch (MI->getOpcode()) {
   default: llvm_unreachable("Unknown SpecialFP instruction!");
   case TargetOpcode::COPY: {
     // We handle three kinds of copies: FP <- FP, FP <- ST, and ST <- FP.
     const MachineOperand &MO1 = MI->getOperand(1);
     const MachineOperand &MO0 = MI->getOperand(0);
     bool KillsSrc = MI->killsRegister(MO1.getReg());
 
while (FPKills) {
       if (isLive(FPReg))
         freeStackSlotAfter(Inst, FPReg);
       FPKills &= ~(1U << FPReg);
     }
 
     // Don't delete the inline asm!
     return;
   }
-
-  case X86::RETQ:
-  case X86::RETL:
-  case X86::RETIL:
-  case X86::RETIQ:
-    // If RET has an FP register use operand, pass the first one in ST(0) and
-    // the second one in ST(1).
-
-    // Find the register operands.
-    unsigned FirstFPRegOp = ~0U, SecondFPRegOp = ~0U;
-    unsigned LiveMask = 0;
-
-    for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
-      MachineOperand &Op = MI->getOperand(i);
-      if (!Op.isReg() || Op.getReg() < X86::FP0 || Op.getReg() > X86::FP6)
-        continue;
-      // FP Register uses must be kills unless there are two uses of the same
-      // register, in which case only one will be a kill.
-      assert(Op.isUse() &&
-             (Op.isKill() ||                        // Marked kill.
-              getFPReg(Op) == FirstFPRegOp ||       // Second instance.
-              MI->killsRegister(Op.getReg())) &&    // Later use is marked kill.
-             "Ret only defs operands, and values aren't live beyond it");
-
-      if (FirstFPRegOp == ~0U)
-        FirstFPRegOp = getFPReg(Op);
-      else {
-        assert(SecondFPRegOp == ~0U && "More than two fp operands!");
-        SecondFPRegOp = getFPReg(Op);
-      }
-      LiveMask |= (1 << getFPReg(Op));
-
-      // Remove the operand so that later passes don't see it.
-      MI->RemoveOperand(i);
-      --i;
-      --e;
-    }
-
-    // We may have been carrying spurious live-ins, so make sure only the
-    // returned registers are left live.
-    adjustLiveRegs(LiveMask, MI);
-    if (!LiveMask) return;  // Quick check to see if any are possible.
-
-    // There are only four possibilities here:
-    // 1) we are returning a single FP value.  In this case, it has to be in
-    //    ST(0) already, so just declare success by removing the value from the
-    //    FP Stack.
-    if (SecondFPRegOp == ~0U) {
-      // Assert that the top of stack contains the right FP register.
-      assert(StackTop == 1 && FirstFPRegOp == getStackEntry(0) &&
-             "Top of stack not the right register for RET!");
-
-      // Ok, everything is good, mark the value as not being on the stack
-      // anymore so that our assertion about the stack being empty at end of
-      // block doesn't fire.
-      StackTop = 0;
-      return;
-    }
-
-    // Otherwise, we are returning two values:
-    // 2) If returning the same value for both, we only have one thing in the FP
-    //    stack.  Consider:  RET FP1, FP1
-    if (StackTop == 1) {
-      assert(FirstFPRegOp == SecondFPRegOp && FirstFPRegOp == getStackEntry(0)&&
-             "Stack misconfiguration for RET!");
-
-      // Duplicate the TOS so that we return it twice.  Just pick some other FPx
-      // register to hold it.
-      unsigned NewReg = ScratchFPReg;
-      duplicateToTop(FirstFPRegOp, NewReg, MI);
-      FirstFPRegOp = NewReg;
-    }
-
-    /// Okay we know we have two different FPx operands now:
-    assert(StackTop == 2 && "Must have two values live!");
-
-    /// 3) If SecondFPRegOp is currently in ST(0) and FirstFPRegOp is currently
-    ///    in ST(1).  In this case, emit an fxch.
-    if (getStackEntry(0) == SecondFPRegOp) {
-      assert(getStackEntry(1) == FirstFPRegOp && "Unknown regs live");
-      moveToTop(FirstFPRegOp, MI);
-    }
-
-    /// 4) Finally, FirstFPRegOp must be in ST(0) and SecondFPRegOp must be in
-    /// ST(1).  Just remove both from our understanding of the stack and return.
-    assert(getStackEntry(0) == FirstFPRegOp && "Unknown regs live");
-    assert(getStackEntry(1) == SecondFPRegOp && "Unknown regs live");
-    StackTop = 0;
-    return;
   }
 
   Inst = MBB->erase(Inst);  // Remove the pseudo instruction
 
   // We want to leave I pointing to the previous instruction, but what if we
   // just erased the first instruction?
   if (Inst == MBB->begin()) {
     DEBUG(dbgs() << "Inserting dummy KILL\n");

test/CodeGen/X86/x86-32-intrcc.ll

 ; RUN: llc -mtriple=i686-unknown-unknown < %s | FileCheck %s
 ; RUN: llc -mtriple=i686-unknown-unknown -O0 < %s | FileCheck %s -check-prefix=CHECK0
 
 %struct.interrupt_frame = type { i32, i32, i32, i32, i32 }
 
[e-mail] = appending global [3 x i8*] [i8* bitcast (void (%struct.interrupt_frame*)* @test_isr_no_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i32)* @test_isr_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i32)* @test_isr_clobbers to i8*)], section "llvm.metadata"
[e-mail] = appending global [4 x i8*] [i8* bitcast (void (%struct.interrupt_frame*)* @test_isr_no_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i32)* @test_isr_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i32)* @test_isr_clobbers to i8*), i8* bitcast (void (%struct.interrupt_frame*)* @test_isr_x87 to i8*)], section "llvm.metadata"
 
 ; Spills eax, putting original esp at +4.
 ; No stack adjustment if declared with no error code
 define x86_intrcc void @test_isr_no_ecode(%struct.interrupt_frame* %frame) {
   ; CHECK-LABEL: test_isr_no_ecode:
   ; CHECK: pushl %eax
   ; CHECK: movl 12(%esp), %eax
   ; CHECK: popl %eax
define x86_intrcc void @test_isr_clobbers(%struct.interrupt_frame* %frame, i32 %ecode) {
   ; CHECK0-SSE-NEXT: popl %eax
   ; CHECK0-SSE-NEXT: popl %ebx
   ; CHECK0-SSE-NEXT: popl %ebp
   ; CHECK0-SSE-NEXT: addl $4, %esp
   ; CHECK0-SSE-NEXT: iretl
   ret void
 }
 
+@f80 = common global x86_fp80 0xK00000000000000000000, align 4
+
+; Test that the presence of x87 does not crash the FP stackifier
+define x86_intrcc void @test_isr_x87(%struct.interrupt_frame* %frame) {
+entry:
+  %0 = load x86_fp80, x86_fp80* @f80, align 4
+  %add = fadd x86_fp80 %0, 0xK3FFF8000000000000000
+  store x86_fp80 %add, x86_fp80* @f80, align 4
+  ret void
+}

test/CodeGen/X86/x86-64-intrcc.ll

 ; RUN: llc -mtriple=x86_64-unknown-unknown < %s | FileCheck %s
 ; RUN: llc -mtriple=x86_64-unknown-unknown -O0 < %s | FileCheck %s -check-prefix=CHECK0
 
 %struct.interrupt_frame = type { i64, i64, i64, i64, i64 }
 
[e-mail] = appending global [3 x i8*] [i8* bitcast (void (%struct.interrupt_frame*)* @test_isr_no_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i64)* @test_isr_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i64)* @test_isr_clobbers to i8*)], section "llvm.metadata"
[e-mail] = appending global [4 x i8*] [i8* bitcast (void (%struct.interrupt_frame*)* @test_isr_no_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i64)* @test_isr_ecode to i8*), i8* bitcast (void (%struct.interrupt_frame*, i64)* @test_isr_clobbers to i8*), i8* bitcast (void (%struct.interrupt_frame*)* @test_isr_x87 to i8*)], section "llvm.metadata"
 
 ; Spills rax, putting original esp at +8.
 ; No stack adjustment if declared with no error code
 define x86_intrcc void @test_isr_no_ecode(%struct.interrupt_frame* %frame) {
   ; CHECK-LABEL: test_isr_no_ecode:
   ; CHECK: pushq %rax
   ; CHECK: movq 24(%rsp), %rax
   ; CHECK: popq %rax
define x86_intrcc void @test_isr_clobbers(%struct.interrupt_frame* %frame, i64 %ecode) {
   ; CHECK0-SSE-NEXT: movaps %xmm0
   ; CHECK0-SSE-NEXT: popq %rbx
   ; CHECK0-SSE-NEXT: popq %rbp
   ; CHECK0-SSE-NEXT: popq %r11
   ; CHECK0-SSE-NEXT: popq %rax
   ; CHECK0-SSE-NEXT: addq $8, %rsp
   ; CHECK0-SSE-NEXT: iretq
   ret void
-}
+}
+
+@f80 = common global x86_fp80 0xK00000000000000000000, align 4
+
+; Test that the presence of x87 does not crash the FP stackifier
+define x86_intrcc void @test_isr_x87(%struct.interrupt_frame* %frame) {
+entry:
+  %0 = load x86_fp80, x86_fp80* @f80, align 4
+  %add = fadd x86_fp80 %0, 0xK3FFF8000000000000000
+  store x86_fp80 %add, x86_fp80* @f80, align 4
+  ret void
+}