[PATCH] D70800: Fix AArch64 AAPCS frame record chain

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Functionally the patch is looking good, nearly there!



================
Comment at: llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h:129
 
+  // Offset from SP-at-entry to frame record (i.e. the spilled frame pointer
+  // and link address).
----------------
logan wrote:
> sdesmalen wrote:
> > Is this description correct? The current meaning of `FrameRecordOffset` seems to be the offset from SP _after_ allocating the callee-save area.
> Thanks.  Reworded.
Thanks! Is it worth renaming the variable to something like `OffsetToFrameRecordFromCalleeSaveBase` to make the meaning of the variable easier to understand in the places it is used? (I couldn't think of something shorter :))


================
Comment at: llvm/test/CodeGen/AArch64/framelayout-frame-record.ll:12
+
+; CHECK: stp d9, d8, [sp, #-48]!
+; CHECK: stp x29, x30, [sp, #[[FP_OFFSET:16]]]
----------------
This has some implicit knowledge that d9 and d8 will be used to keep d0 and d1.
This is probably better tested with a MIR test like:

```#RUN: llc -mtriple=aarch64-- -start-before prologepilog %s -o - | FileCheck %s
---
name: TestFrameRecordLocation
tracksRegLiveness: true
frameInfo:
  isFrameAddressTaken: true
body: |
  bb.0:
    $d8 = IMPLICIT_DEF
    $d9 = IMPLICIT_DEF
    RET_ReallyLR
...
```
which explicitly marks d8 and d9 as callee-saved. The `isFrameAddressTaken: true` will trigger the use of the frame-pointer, and thus storing of the frame-record. You only need to add a few check lines.


================
Comment at: llvm/test/CodeGen/AArch64/framelayout-frame-record.ll:13
+; CHECK: stp d9, d8, [sp, #-48]!
+; CHECK: stp x29, x30, [sp, #[[FP_OFFSET:16]]]
+; CHECK: str x19, [sp, #32]
----------------
nit: instead of checking `[[FP_OFFSET:16]]` as a variable, you can just check `16` directly.


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

https://reviews.llvm.org/D70800