[PATCH] D70800: Fix AArch64 AAPCS frame record chain

[Logan Chien via Phabricator via llvm-commits]

logan marked an inline comment as done.
logan added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2289
+      AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
+      if (Reg1 == AArch64::FP)
+        AFI->setFrameRecordOffset((RPI.Offset + 1) * Size);
----------------
sdesmalen wrote:
> logan wrote:
> > sdesmalen wrote:
> > > There is a condition here that is not yet tested. If the frame-record is saved that is both LR and FP, not just FP, so is this case needed?
> > I think we don't have to check LR here because AAPCS64 guarantees FP and LR will be spilled to consecutive words.  Besides, we only care about the address of the spilled FP.
> If `LR` is also spilled, shouldn't `isPaired()` be true? This is testing two layouts `(LR, FP)` and `(FP, <something else>)`, where the latter is not a frame-record.
> 
> This makes me think that this code also needs a condition for `hasFP(MF)`, because that guarantees the existence of a frame-record (as opposed to ordinary spills of FP/LR that don't necessarily constitute the framerecord)
I am concerned about the case that FP is the second register in the pair (next one).  Since the `EmitMI` function only gets a `RegisterPairInfo`.  I cannot see the next `RegisterPairInfo`.  Let me think about how to revise this tomorrow.

`hasFP(MF)` makes sense to me.  I'll update the code in the next revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70800