[PATCH] D50288: [ARM64] [Windows] Exception handling, part #2

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:184
+// A small RAII class to handle insertion of SEH codes on multiple paths.
+class InsertCFI {
+  MachineBasicBlock &MBB;
----------------
>From what I've seen in the way it's used, I'm not sure this class is actually useful. It seems to be always constructed at the end of a scope, where a `BuildMI` would have been much more readable.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:446
 
+static unsigned getRegNumX(unsigned Reg) {
+  if (Reg == AArch64::FP)
----------------
Can you use `MCRegisterInfo::getDwarfRegNum` for this?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:732
+  if (NeedsWinCFI) {
+    auto SEH_MI = std::next(MachineBasicBlock::iterator(MI));
+    auto &MBB = *(MI.getParent());
----------------
Can you move this into a separate function?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:818
 
+  // The very laset FrameSetup instruction  indicates the end of prologue.  Emit
+  // a SEH opcode indicating the prologue end.
----------------
Typo (`laset`) and extra spaces here.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:825
+      ++LastFrameSetupI;
+    DebugLoc DL_CFI =
+      (MBB.end() == LastFrameSetupI) ? DL : LastFrameSetupI->getDebugLoc();
----------------
`DL_CFI` does not match the LLVM naming conventions.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:879
     NumBytes = 0;
-  } else if (PrologueSaveSize != 0) {
+  } else if (AFI->getCalleeSavedStackSize() != 0) {
     MBBI = convertCalleeSaveRestoreToSPPrePostIncDec(MBB, MBBI, DL, TII,
----------------
This does not seem equivalent to what `PrologueSaveSize` was.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:899
+  // The code below is not applicable to funclets.  We have emitted all the
+  // SEH opcodes that we needed to emit.  The FP and BP belong to the
+  // containing function.
----------------
Extra spaces in the comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1196
   auto PrologueSaveSize = AFI->getCalleeSavedStackSize() + FixedObject;
+  AFI->setLocalStackSize(NumBytes - PrologueSaveSize);
   bool CombineSPBump = shouldCombineCSRLocalStackBump(MF, NumBytes);
----------------
This was already set in `emitPrologue`. Why is it needed here?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1200
 
-  if (!CombineSPBump && PrologueSaveSize != 0) {
+  if (!CombineSPBump && AFI->getCalleeSavedStackSize() != 0) {
     MachineBasicBlock::iterator Pop = std::prev(MBB.getFirstTerminator());
----------------
Same here, it doesn't seem equivalent to `PrologueSaveSize`.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1502
+          (!RPI.IsGPR && AArch64::FPR64RegClass.contains(NextReg))) &&
+          (!NeedsWinCFI || (NextReg == RPI.Reg1 + 1)))
         RPI.Reg2 = NextReg;
----------------
Can we make this windows specific check a separate function and move the comment there?


Repository:
  rL LLVM

https://reviews.llvm.org/D50288