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