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

[Sanjin Sijaric via Phabricator via llvm-commits]

ssijaric added a comment.

Thanks for reviewing, Francis.  I'll upload an updated patch.



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:159
 
+static bool isSEHOpcode(unsigned Opc) {
+  switch (Opc) {
----------------
thegameg wrote:
> Should this be in `AArch64InstrInfo`?
Will move it in the updated patch.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:484
+    assert(false && "No SEH Opcode for this instruction");
+  case AArch64::STPDpre:
+  case AArch64::LDPDpost: {
----------------
thegameg wrote:
> How about:
> 
> ```
> case AArch64::STPDpost:
>   Imm = -Imm;
>   LLVM_FALLTHROUGH
> case AArch64::STPDpre:
>   [...]
> ```
> 
> Same applied to the other cases below.
Will change it in the updated patch.


================
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)
----------------
thegameg wrote:
> Is `Reg0 - AArch64::D0` the same as `MRI->getDwarfRegNum`?
> 
> Same applied to the other cases below.
getDwarfRegNum doesn't work for floating point registers, as the Dwarf register numbers start at 64 for those:

e.g.

def B0    : AArch64Reg<0,   "b0">, DwarfRegNum<[64]>;

I looked around for an existing API to retrieve the number corresponding to the register, but didn't see one.




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


================
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
----------------
thegameg wrote:
> Indentation seems off.
Will fix in the updated patch.


================
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));
----------------
thegameg wrote:
> 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.
I'll special case the whole block in the updated patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D50288