[PATCH] D71754: [AArch64] Fix save register pairing for Windows AAPCS

[Renato Golin via Phabricator via llvm-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1877
   // the MCLayer.  We need to add support for the save_lrpair unwind code.
+  if (Reg2 == AArch64::FP)
+    return true;
----------------
sanwou01 wrote:
> rengolin wrote:
> > This seems awfully permissive. Shouldn't you check that the pairs are explicitly:
> > 
> >     FP + LR
> >     LR + FP
> Hm, this invalidates any pair with FP as the second register, so it follows that FP will be the first register of a pair (on Windows/WinAAPCS). The assert below (line 2028) checks that when FP is the first register of a pair, LR is the second.
> 
> Maybe I misunderstood your comment, but that seems quite precise to me!
Right, my comment was about the assert below, but I guess you're right. The code that uses this function is not that trivial to read. :)


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1986
         if (AArch64::GPR64RegClass.contains(NextReg) &&
-            !invalidateRegisterPairing(RPI.Reg1, NextReg, NeedsWinCFI,
+            !invalidateRegisterPairing(RPI.Reg1, NextReg, IsWindows, NeedsWinCFI,
                                        NeedsFrameRecord))
----------------
sanwou01 wrote:
> rengolin wrote:
> > Is Windows' only ABI WindowsAAPCS?
> Good point. It might be so in the AArch64 backend, but who knows when a new ABI comes around.
Right, we change that when it does. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71754