[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:1863
   // to combine multiple loads / stores.
-  bool CanEliminateFrame = true;
+  bool CanEliminateFrame = !requiresAAPCSFrameRecord(MF);
   bool CS1Spilled = false;
----------------
Should this also check hasFP?


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:2110
+      if ((requiresAAPCSFrameRecord(MF) ||
+           MF.getTarget().Options.DisableFramePointerElim(MF)) &&
+          !LRSpilled) {
----------------
pratlucas wrote:
> efriedma wrote:
> > 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.
> For cases where a function's codegen requires the use of a frame pointer, we want an AAPCS compliant frame record to be generated even when the frame pointer elimination is enabled. For that reason we need the two options to be orthogonal on some level.
> I've updated this check to be more strict and only consider `requiresAAPCSFrameRecord()` when the function has a frame pointer.
> For cases where a function's codegen requires the use of a frame pointer, we want an AAPCS compliant frame record to be generated even when the frame pointer elimination is enabled.

That's... hmm.  Feels a little weird, but I guess it makes sense.

The `hasFP(MF)` here is redundant; this is already inside an `if (HasFP) {` block.


================
Comment at: llvm/lib/Target/ARM/Thumb1FrameLowering.cpp:1167
+                   STI.hasV5TOps());
+  // Only unused return registers can be used as copy regs at this point
+  popRegsFromStack(MBB, MI, TII, FrameRecord, UnusedReturnRegs, IsVarArg,
----------------
Are we actually guaranteed to have any unused return registers at this point?  The calling convention uses r0-r3 to return values.


================
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
----------------
pratlucas wrote:
> efriedma wrote:
> > Can we use sp-relative accesses here?  If we're not doing dynamic allocations, it should be a fixed offset.
> The current criteria for the Arm and Thumb backends is that fixed  frame indices are always accessed through FP whenever it is available (See [[ https://github.com/llvm/llvm-project/blob/aed49eac87b8aa77298252ea781bae4725ae8046/llvm/lib/Target/ARM/ARMFrameLowering.cpp#L1083 | ARMFrameLowering.cpp ]]).
> I guess that could be changed, but I feel it would fall outside the scope of this patch.
This patch does make it much more likely that fp-relative accesses are going to be out of range, but sure, I guess we can treat it as a separate issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125094