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