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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei marked 2 inline comments as done and an inline comment as not done.
pengfei added a comment.

Thanks for the patient review and valuable suggestions.



================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1397-1398
+          else
+            Offset = getFrameIndexReference(MF, FI, IgnoredFrameReg) +
+                     SEHFrameOffset;
 
----------------
rnk wrote:
> 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.
The return value of `getFrameIndexReferenceSP(MF, FI, IgnoredFrameReg, SEHFrameOffset)` has 8 bytes gap as against the former math. It may need more code to eliminate the gap. So it's better to keep it.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1862
+  FrameReg = TRI->getStackRegister();
+  return 32 + XMMSpillSlotOffset - MFI.getObjectOffset(FI);
+}
----------------
rnk wrote:
> 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()`?
Because we always need condition `IsFunclet` to select this method in caller `emitPrologue`.
I changed the function name to `getWin64EHFrameIndexRef`, which may be more appropriate.

Right, `MFI.getMaxCallFrameSize()` is more robust.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2042
     // ensure alignment
-    SpillSlotOffset -= std::abs(SpillSlotOffset) % Align;
+    int Remainder = SpillSlotOffset % Align;
+    if (Remainder < 0)
----------------
rnk wrote:
> 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);
The former code doesn't align it in some cases, e.g. `SpillSlotOffset` = -8 and `Align` = 64.

Yes, it can be simplified to this. Thanks!


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.h:41
+  /// in bytes.
+  int XMMSpillSlotOffset = 0;
+
----------------
rnk wrote:
> Perhaps initialize it to INT32_MIN for safety?
I have used a FI->Slot map to replace this. It's more robust and flexible.


================
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);
----------------
rnk wrote:
> 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.
You are correct. Thanks for the reminding!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66596