[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:159
 
+static bool isSEHOpcode(unsigned Opc) {
+  switch (Opc) {
----------------
Should this be in `AArch64InstrInfo`?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:484
+    assert(false && "No SEH Opcode for this instruction");
+  case AArch64::STPDpre:
+  case AArch64::LDPDpost: {
----------------
How about:

```
case AArch64::STPDpost:
  Imm = -Imm;
  LLVM_FALLTHROUGH
case AArch64::STPDpre:
  [...]
```

Same applied to the other cases below.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:491
+    MIB = BuildMI(MF, DL, TII.get(AArch64::SEH_SaveFRegP_X))
+              .addImm(Reg0 - AArch64::D0)
+              .addImm(Reg1 - AArch64::D0)
----------------
Is `Reg0 - AArch64::D0` the same as `MRI->getDwarfRegNum`?

Same applied to the other cases below.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:508
+                .setMIFlag(Flag);
+    else {
+      MIB = BuildMI(MF, DL, TII.get(AArch64::SEH_SaveRegP_X))
----------------
Why the extra `{` / `}`?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1440
+static bool invalidateWindowsRegisterPairing(unsigned Reg1, unsigned Reg2,
+                                          bool NeedsWinCFI) {
+  // If we are generating register pairs for a Windows function that requires
----------------
Indentation seems off.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1629
+    // Windows unwinding codes require that gprs be consecutive if they are paired.
+    if (NeedsWinCFI) {
+      MIB.addReg(Reg1, getPrologueDeath(MF, Reg1));
----------------
I would rather just special-case the whole block since it seems to be having a pretty different behaviour than the non-wincfi path, but it's just a personal preference.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1706
+
+    if (NeedsWinCFI) {
+      MIB.addReg(Reg1, getDefRegState(true));
----------------
Same here.


Repository:
  rL LLVM

https://reviews.llvm.org/D50288