[PATCH] D66596: [WinEH] Allocate space in funclets stack to save XMM CSRs

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks, this seems more straightforward. :)



================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1397-1398
+          else
+            Offset = getFrameIndexReference(MF, FI, IgnoredFrameReg) +
+                     SEHFrameOffset;
 
----------------
I think this `else` can be simplified by using `getFrameIndexReferenceSP`. The .seh_savexmm directive expects an offset relative to SP-after-prologue, but getFrameIndexReference may use an RBP offset, and the SEHFrameOffset is the difference between them at this point. Making the preference for RSP offsets explicit seems clearer.

At first I wanted to find a way to eliminate the need for the if/else here, but I see it's not possible without a "prefer SP" variant to the new XMM frame index ref helper.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1862
+  FrameReg = TRI->getStackRegister();
+  return 32 + XMMSpillSlotOffset - MFI.getObjectOffset(FI);
+}
----------------
This calculation is only correct in funclets. I think what I had in mind was that we'd pass in `IsFunclet` to this method, since all the callers have to calculate it anyway. That way we avoid one more conditional at the call site.

Is 32 always correct? Shouldn't it be `MFI.getMaxCallFrameSize()`?


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2042
     // ensure alignment
-    SpillSlotOffset -= std::abs(SpillSlotOffset) % Align;
+    int Remainder = SpillSlotOffset % Align;
+    if (Remainder < 0)
----------------
What is the desired change in behavior here? It looks like this can be simplified. SpillSlotOffset is always negative and Align is always positive, so I would expect Remainder to always be zero or negative.

Would this expression calculate the same value?
  SpillSlotOffset = -alignTo(-SpillSlotOffset, Align);


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2054
+
+    // Save the start offset and size of XMM in stack frame for funlets.
+    if (X86::VR128RegClass.contains(Reg)) {
----------------
"for funclets"


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:41
+  /// in bytes.
+  int XMMSpillSlotOffset = 0;
+
----------------
Perhaps initialize it to INT32_MIN for safety?


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:713
     FIOffset = TFI->getFrameIndexReferenceSP(MF, FrameIndex, BasePtr, 0);
+  } else if (TFI->Is64Bit && MBB.isEHFuncletEntry()) {
+    FIOffset = TFI->getSaveXMMFrameIndexRef(MF, FrameIndex, BasePtr);
----------------
I would expect that for a multi-bb cleanup funclet, `MBB.isEHFuncletEntry()` wouldn't be sufficient, and you'd need to check if the terminator is a funclet return.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66596/new/

https://reviews.llvm.org/D66596