[WinEH] Allocate unique stack slots for xmm CSRs in funclets

[andrew.w.kaylor]

Summary

Currently funclets reuse the same stack slots that are used in the parent function for saving callee-saved xmm registers. If the parent function modifies a callee-saved xmm register before an excpetion is thrown, the catch handler will overwrite the original saved value.

This patch allocates a second stack slot to be used in the EH funclets for saving these same registers. Long term, it would be better to determine actual CSR use by the funclets and only allocate the extra space when needed.

[rnk]

test/CodeGen/X86/catchpad-realign-savexmm.ll

65–66

I don't think this will work long in practice because we are storing XMM into the parent function's stack frame, but the .seh_savexmm directive describes locations relative the the funclet's RSP. So, when the stack is unwound, (throw an exception out of a catch block) XMM CSRs will not be restored correctly.

Another idea I had for fixing this was to change X86FrameLowering::getFrameIndexReference to do something special for XMM CSRs (we should be able to find the list of them somewhere), and resolve them to some SP-relative offset in the funclet's frame. We'll have to adjust the "SUB RSP, 32" that we currently emit for every funclet as well for that to work.

[andrew.w.kaylor]

test/CodeGen/X86/catchpad-realign-savexmm.ll

65–66

I see what you mean, and that makes the hardcoded offset suggestion make more sense.

That said, I think this would probably "work" (i.e. not fail) for a lot of cases. If the funclet throws an exception the unwinder will put garbage in the CSRs, but if the exception isn't caught by the parent function the correct value will be filled in when the parent's stack unwinds. If my thinking on this is correct, we'd really only keep incorrect values when a funclet throws an exception and the parent catches it. I'm not suggesting that this is in any way good, but it's slightly less bad than what we're doing now.

I'll see if I can figure out how to do it the way you are suggesting. In the meantime do you think what I have here is possibly useful as a workaround for desperate people (of which I am currently one)? And if so, do you think it's worth committing if I'm not going to have a proper fix soon?

[andrew.w.kaylor]

I haven't had a lot more time to spend on this, but I did do a bit of investigation. I still only have a vague idea of what would be required to implement hard-coded offsets, but it didn't look to me like there is existing infrastructure for this so all of the things that just kind of magically worked with this patch (like growing the stack allocation and writing the .seh_savexmm directive) would need to be explicitly updated, right?

On the other hand, it looked like it might be relatively easy to update the code that writes the savexmm directive to describe the actual location of the stack slot I'm using in the current version of this patch. If I was reading the code correctly, it looks like it's already using the offset between the parent stack pointer and the funclet stack pointer to figure out how to build the instruction that actually stores the register to the stack. So we should be able to do a similar calculation to fill out the savexmm directive correctly. Does that seem worth exploring?

[rnk]

In D57970#1397522, @andrew.w.kaylor wrote:

I haven't had a lot more time to spend on this, but I did do a bit of investigation. I still only have a vague idea of what would be required to implement hard-coded offsets, but it didn't look to me like there is existing infrastructure for this so all of the things that just kind of magically worked with this patch (like growing the stack allocation and writing the .seh_savexmm directive) would need to be explicitly updated, right?

On the other hand, it looked like it might be relatively easy to update the code that writes the savexmm directive to describe the actual location of the stack slot I'm using in the current version of this patch. If I was reading the code correctly, it looks like it's already using the offset between the parent stack pointer and the funclet stack pointer to figure out how to build the instruction that actually stores the register to the stack. So we should be able to do a similar calculation to fill out the savexmm directive correctly. Does that seem worth exploring?

I don't think it's possible, because the XMM registers are stored relative to RBP, which points into the stack frame of the parent function. I believe the savexmm directive offsets are always relative to the RSP at the end of the prologue, even if a frame pointer is in use, and that points into the frame of the funclet, which is usually empty (we always use a frame size of 32 or something).

Anyway, I think you are right that this intermediate state would fix the issue for a lot of folks. Typically you don't need to restore XMM registers to unwind the stack (it's OK if their contents are wrong), so this is only a problem if someone recovers from an exception thrown out of a catch block.

[andrew.w.kaylor]

I haven't had time to work on the alternate approach suggested, but one of my colleagues is working on it.

The patch as I have it here has been working very well in our internal testing, but I hope we'll have something better to offer in the next couple of weeks.

[andrew.w.kaylor]

This was fixed in a different way.

include/llvm/CodeGen/MachineFrameInfo.h

 /// Callee saved reg can also be saved to a different register rather than
 /// on the stack by setting DstReg instead of FrameIdx.
 class CalleeSavedInfo {
   unsigned Reg;
   union {
     int FrameIdx;
     unsigned DstReg;
   };
+
+  // Windows exception handling requires callee saved registers to be saved
+  // for EH funclets separately from the main function.
+  int FuncletFrameIdx;
+
   /// Flag indicating whether the register is actually restored in the epilog.
   /// In most cases, if a register is saved, it is also restored. There are
   /// some situations, though, when this is not the case. For example, the
   /// LR register on ARM is usually saved, but on exit from the function its
   /// saved value may be loaded directly into PC. Since liveness tracking of
   /// physical registers treats callee-saved registers are live outside of
   /// the function, LR would be treated as live-on-exit, even though in these
   /// scenarios it is not. This flag is added to indicate that the saved
   /// register described by this object is not restored in the epilog.
   /// The long-term solution is to model the liveness of callee-saved registers
   /// by implicit uses on the return instructions, however, the required
   /// changes in the ARM backend would be quite extensive.
   bool Restored;
   /// Flag indicating whether the register is spilled to stack or another
   /// register.
   bool SpilledToReg;
 
 public:
   explicit CalleeSavedInfo(unsigned R, int FI = 0)
-  : Reg(R), FrameIdx(FI), Restored(true), SpilledToReg(false) {}
+      : Reg(R), FrameIdx(FI), FuncletFrameIdx(0), Restored(true),
+        SpilledToReg(false) {}
 
   // Accessors.
   unsigned getReg()                        const { return Reg; }
   int getFrameIdx()                        const { return FrameIdx; }
   unsigned getDstReg()                     const { return DstReg; }
   void setFrameIdx(int FI) {
     FrameIdx = FI;
     SpilledToReg = false;
   }
   void setDstReg(unsigned SpillReg) {
     DstReg = SpillReg;
     SpilledToReg = true;
   }
+
+  int getFuncletFrameIdx() const { return FuncletFrameIdx; }
+  void setFuncletFrameIdx(int FI) { FuncletFrameIdx = FI; }
+
   bool isRestored()                        const { return Restored; }
   void setRestored(bool R)                       { Restored = R; }
   bool isSpilledToReg()                    const { return SpilledToReg; }
 };
 
 /// The MachineFrameInfo class represents an abstract stack frame until
 /// prolog/epilog code is inserted.  This class is key to allowing stack frame
 /// representation optimizations, such as frame pointer elimination.  It also

lib/Target/X86/X86FrameLowering.cpp

for (unsigned i = CSI.size(); i != 0; --i) {
     SpillSlotOffset -= std::abs(SpillSlotOffset) % Align;
     // spill into slot
     SpillSlotOffset -= Size;
     int SlotIndex = MFI.CreateFixedSpillStackObject(Size, SpillSlotOffset);
     CSI[i - 1].setFrameIdx(SlotIndex);
     MFI.ensureMaxAlignment(Align);
   }
 
+  // If this function has funclets, create extra slots for non-GPRs in the
+  // funclets.
+  if (MF.hasEHFunclets()) {
+    for (unsigned i = CSI.size(); i != 0; --i) {
+      unsigned Reg = CSI[i - 1].getReg();
+      if (X86::GR64RegClass.contains(Reg) || X86::GR32RegClass.contains(Reg))
+        continue;
+
+      // FIXME: I'm not sure all of this is necessary, but it will work.
+
+      // If this is k-register make sure we lookup via the largest legal type.
+      MVT VT = MVT::Other;
+      if (X86::VK16RegClass.contains(Reg))
+        VT = STI.hasBWI() ? MVT::v64i1 : MVT::v16i1;
+
+      const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg, VT);
+      unsigned Size = TRI->getSpillSize(*RC);
+      unsigned Align = TRI->getSpillAlignment(*RC);
+      // ensure alignment
+      SpillSlotOffset -= std::abs(SpillSlotOffset) % Align;
+      // spill into slot
+      SpillSlotOffset -= Size;
+      int SlotIndex = MFI.CreateFixedSpillStackObject(Size, SpillSlotOffset);
+      CSI[i - 1].setFuncletFrameIdx(SlotIndex);
+      MFI.ensureMaxAlignment(Align);
+    }
+  }
+
   return true;
 }
 
 bool X86FrameLowering::spillCalleeSavedRegisters(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
     const std::vector<CalleeSavedInfo> &CSI,
     const TargetRegisterInfo *TRI) const {
   DebugLoc DL = MBB.findDebugLoc(MI);
for (unsigned i = CSI.size(); i != 0; --i) {
     MVT VT = MVT::Other;
     if (X86::VK16RegClass.contains(Reg))
       VT = STI.hasBWI() ? MVT::v64i1 : MVT::v16i1;
 
     // Add the callee-saved register as live-in. It's killed at the spill.
     MBB.addLiveIn(Reg);
     const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg, VT);
 
-    TII.storeRegToStackSlot(MBB, MI, Reg, true, CSI[i - 1].getFrameIdx(), RC,
-                            TRI);
+    int FrameIdx = (MBB.isEHFuncletEntry() ? CSI[i - 1].getFuncletFrameIdx()
+                                           : CSI[i - 1].getFrameIdx());
+    TII.storeRegToStackSlot(MBB, MI, Reg, true, FrameIdx, RC, TRI);
     --MI;
     MI->setFlag(MachineInstr::FrameSetup);
     ++MI;
   }
 
   return true;
 }
 

 }
 
 bool X86FrameLowering::restoreCalleeSavedRegisters(MachineBasicBlock &MBB,
                                                MachineBasicBlock::iterator MI,
                                           std::vector<CalleeSavedInfo> &CSI,
                                           const TargetRegisterInfo *TRI) const {
   if (CSI.empty())
     return false;
-
-  if (MI != MBB.end() && isFuncletReturnInstr(*MI) && STI.isOSWindows()) {
+  bool IsEHFuncletReturn =
+      (MI != MBB.end() && isFuncletReturnInstr(*MI) && STI.isOSWindows());
+  if (IsEHFuncletReturn) {
     // Don't restore CSRs in 32-bit EH funclets. Matches
     // spillCalleeSavedRegisters.
     if (STI.is32Bit())
       return true;
     // Don't restore CSRs before an SEH catchret. SEH except blocks do not form
     // funclets. emitEpilogue transforms these to normal jumps.
     if (MI->getOpcode() == X86::CATCHRET) {
       const Function &F = MBB.getParent()->getFunction();
if (X86::GR64RegClass.contains(Reg) ||
       continue;
 
     // If this is k-register make sure we lookup via the largest legal type.
     MVT VT = MVT::Other;
     if (X86::VK16RegClass.contains(Reg))
       VT = STI.hasBWI() ? MVT::v64i1 : MVT::v16i1;
 
     const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg, VT);
-    TII.loadRegFromStackSlot(MBB, MI, Reg, CSI[i].getFrameIdx(), RC, TRI);
+    int FrameIdx = (IsEHFuncletReturn ? CSI[i].getFuncletFrameIdx()
+                                  : CSI[i].getFrameIdx());
+    TII.loadRegFromStackSlot(MBB, MI, Reg, FrameIdx, RC, TRI);
   }
 
   // POP GPRs.
   unsigned Opc = STI.is64Bit() ? X86::POP64r : X86::POP32r;
   for (unsigned i = 0, e = CSI.size(); i != e; ++i) {
     unsigned Reg = CSI[i].getReg();
     if (!X86::GR64RegClass.contains(Reg) &&
         !X86::GR32RegClass.contains(Reg))

test/CodeGen/X86/catchpad-realign-savexmm.ll

 catch:
   %p = catchpad within %cs1 [i8* null, i32 64, i8* null]
   catchret from %p to label %return
 }
 
 ; CHECK: f: # @f
 ; CHECK: pushq   %rbp
 ; CHECK: .seh_pushreg 5
-; CHECK: subq    $64, %rsp
-; CHECK: .seh_stackalloc 64
-; CHECK: leaq    64(%rsp), %rbp
-; CHECK: .seh_setframe 5, 64
+; CHECK: subq    $80, %rsp
+; CHECK: .seh_stackalloc 80
+; CHECK: leaq    80(%rsp), %rbp
+; CHECK: .seh_setframe 5, 80
 ; CHECK: movaps  %xmm6, -16(%rbp)        # 16-byte Spill
-; CHECK: .seh_savexmm 6, 48
+; CHECK: .seh_savexmm 6, 64
 ; CHECK: .seh_endprologue
-; CHECK: movq    $-2, -24(%rbp)
+; CHECK: movq    $-2, -40(%rbp)
 ; CHECK: movsd   fp_global(%rip), %xmm6  # xmm6 = mem[0],zero
 ; CHECK: callq   g
 ; CHECK: addsd   __real@3ff0000000000000(%rip), %xmm6
 ; CHECK: movsd   %xmm6, fp_global(%rip)
 ; CHECK: .Ltmp{{.*}}
 ; CHECK: callq   g
 ; CHECK: .Ltmp{{.*}}
 ; CHECK: .LBB{{.*}} # Block address taken
 ; CHECK: movaps  -16(%rbp), %xmm6
-; CHECK: addq    $64, %rsp
+; CHECK: addq    $80, %rsp
 ; CHECK: popq    %rbp
 ; CHECK: retq
 ; CHECK: .seh_handlerdata
+
+; CHECK: "?catch$2@?0?f@4HA":
+; CHECK: .seh_proc "?catch$2@?0?f@4HA"
+; CHECK:    .seh_handler __CxxFrameHandler3, @unwind, @except
+; CHECK: .LBB0{{.*}} # %catch
+; CHECK:    movq  %rdx, 16(%rsp)
+; CHECK:    pushq %rbp
+; CHECK:  .seh_pushreg 5
+; CHECK:  subq  $32, %rsp
+; CHECK:  .seh_stackalloc 32
+; CHECK:  leaq  80(%rdx), %rbp
+; CHECK:  movapd  %xmm6, -32(%rbp)        # 16-byte Spill
+; CHECK:  .seh_savexmm 6, 48
+; CHECK:  .seh_endprologue
+; CHECK:  movapd  -32(%rbp), %xmm6        # 16-byte Reload
+; CHECK:  leaq  .LBB{{.*}}(%rip), %rax
+; CHECK:  addq  $32, %rsp
+; CHECK:  popq  %rbp
+; CHECK:  retq                            # CATCHRET