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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
