[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:1844
+    for (const auto &MI : MBB)
+      if (MI.getOpcode() == ARM::tSTRspi || MI.getOpcode() == ARM::tSTRi)
+        for (const auto &Op : MI.operands())
----------------
How do you end up with tSTRi with a frameindex operand?  SelectionDAG won't generate that, I think.


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:2110
+      if ((requiresAAPCSFrameRecord(MF) ||
+           MF.getTarget().Options.DisableFramePointerElim(MF)) &&
+          !LRSpilled) {
----------------
Should requiresAAPCSFrameRecord() be orthogonal to DisableFramePointerElim()?  I mean, we have a complete set of flags controlling frame pointer elimination; the only part that's "new" here is that the frame pointer is stored in r11, instead of r7.


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:2317
+    if ((BigFrameOffsets || canSpillOnFrameIndexAccess(MF, *this)) &&
+        !ExtraCSSpill) {
       // If any non-reserved CS register isn't spilled, just spill one or two
----------------
Rearrange this to check ExtraCSSpill first, so we don't run canSpillOnFrameIndexAccess() if it isn't necessary?


================
Comment at: llvm/test/CodeGen/Thumb/frame-access.ll:73
+; CHECK-AAPCS: mov r0, r11
+; CHECK-AAPCS: str r1, [r0, #8]
+; CHECK-AAPCS: mov r0, r11
----------------
Can we use sp-relative accesses here?  If we're not doing dynamic allocations, it should be a fixed offset.


================
Comment at: llvm/test/CodeGen/Thumb/frame-access.ll:96
+; CHECK-ATPCS:       add r7, sp, #8
+; CHECK-AAPCS:       add r11, sp, #0
 ; Align stack
----------------
I don't think "add r11, sp, #0" corresponds to a legal Thumb1 instruction?  (Does -verify-machine-instrs catch this?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125094