[PATCH] D50288: [ARM64] [Windows] Exception handling support in frame lowering

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Test coverage seems reasonable to me.



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:567
+  default:
+    assert(false && "Fix the offset in the SEH instruction");
+    break;
----------------
llvm_unreachable.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:884
 
     BuildMI(MBB, MBBI, DL, TII->get(AArch64::MOVi64imm), AArch64::X15)
         .addImm(NumWords)
----------------
MOVi64imm is a pseudo-instruction that expands to up to four instructions.  (Here, probably not more than two, given that the unwind opcodes don't allow allocating more than 256MB of stack at a time anyway.)  So I think you might be short one NOP in cases that involve allocating more than 1MB of stack.

Probably the simplest solution is to explicitly write out the relevant instructions in frame lowering, so you know how many nops you will need.  (See AArch64ExpandPseudo::expandMOVImmSimple.)


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1527
 
-    // Add the next reg to the pair if it is in the same register class.
     if (i + 1 < Count) {
       unsigned NextReg = CSI[i + 1].getReg();
----------------
Not sure why you're deleting the comment here.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1584
         RPI.Type != RegPairInfo::FPR128 && !RPI.isPaired()) {
+      FixupDone = NeedsWinCFI;
       Offset -= 8;
----------------
`FixupDone = NeedsWinCFI;` is technically correct, I guess, but it seems simpler to write the equivalent `FixupDone = true;`.

It might be worth changing this logic further at some point, to rearrange spills to avoid ldp/stp operations that aren't 16-byte-aligned on Windows.  (They're not wrong, but they might be slower.)  But maybe it's rare enough that it doesn't matter.


Repository:
  rL LLVM

https://reviews.llvm.org/D50288