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

[Reid Kleckner via Phabricator via llvm-commits]

rnk requested changes to this revision.
rnk added a comment.
This revision now requires changes to proceed.

My suggestion for drastically simplifying this is to modify `X86RegisterInfo::eliminateFrameIndex` to check if it's lowering an XMM CSR slot. In that case, it would call a new method, `X86FrameLowering::getSaveXMMFrameIndexRef`, which would contain the logic to check if the current block is a funclet prologue or epilogue. If the parent BB of the current instruction is in a funclet, then XMM slots would be resolved relative to RSP using the funclet frame size. If not, it would delegate to the standard `getFrameIndexReference` logic, yielding the correct register and offset for XMM slots in non-funclet prologues and epilogues. The prologue code that currently inserts savexmm markers would need to call this new helper as well instead of the standard `getFrameIndexReference` method. Does that plan seem reasonable?



================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1186-1187
     NumBytes = getWinEHFuncletFrameSize(MF);
+    if (IsWin64Prologue)
+      NumBytes += X86FI->getCalleeSavedXMMFrameInfo(XMMFrameSlotOrigin);
+  }
----------------
I think you should include this offset in the return value from `getWinEHFuncletFrameSize`. With your change, `getWinEHFuncletFrameSize` no longer returns the true funclet frame size, and it would be a bug to have a call site that forgets to include the XMM size offset. In fact, I found and fixed this exact bug in the catch funclet establishing frame offset code. We should fix it by design before relanding.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2237
     unsigned Reg = CSI[i].getReg();
-    if (X86::GR64RegClass.contains(Reg) ||
-        X86::GR32RegClass.contains(Reg))
-      continue;
+    if (MBB.isEHFuncletEntry() && STI.is64Bit()) {
+      if (X86::VR128RegClass.contains(Reg)) {
----------------
For a multi-bb funclet, won't this be false? You should reuse the `isFuncletReturnInstr` check from above to know if this is a funclet epilogue.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2239
+      if (X86::VR128RegClass.contains(Reg)) {
+        int Offset = (CSI[i].getFrameIdx() - XMMFrameSlotOrigin - 1) * 16;
+        addRegOffset(BuildMI(MBB, MI, DL,
----------------
I don't like this repeated math that has to be kept in sync between the prologue and epilogue. If we could factor it out, I would find it easier to read and have more confidence in the correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63396