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

[Lucas Prates via Phabricator via cfe-commits]

pratlucas marked 4 inline comments as done.
pratlucas 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())
----------------
efriedma wrote:
> How do you end up with tSTRi with a frameindex operand?  SelectionDAG won't generate that, I think.
Indeed I didn't hit any case where tSTRi was emitted during my tests, but I included it here to be on the safe side given the contents of the comment on line 2050. I'm happy to remove it if it's indeed not necessary.


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:2110
+      if ((requiresAAPCSFrameRecord(MF) ||
+           MF.getTarget().Options.DisableFramePointerElim(MF)) &&
+          !LRSpilled) {
----------------
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.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125094