This is an archive of the discontinued LLVM Phabricator instance.

Differential D46777

[ARM] Back up R4 and LR if calling the stack probe function
ClosedPublic

Authored by mstorsjo on May 11 2018, 2:47 PM.

Details

Reviewers
compnerd
peter.smith
t.p.northover
efriedma
javed.absar
Commits
rGace7ae935ffc: [ARM] Back up R4 and LR if calling the stack probe function
rL332298: [ARM] Back up R4 and LR if calling the stack probe function
Summary

At this point in determineCalleeSaves, try to get a first estimate of the number of register that will be backed up, to estimate whether a stack probe is going to be needed later or not.

I ran into a case with a function that wouldn't otherwise back up R4 at all, clobbering it with the stack probe call. And apparently one existing test case forgot to back up LR even though it calls the chkstk function within the test.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.May 11 2018, 2:47 PM
Herald added a reviewer: javed.absar. · View Herald TranscriptMay 11 2018, 2:47 PM
Herald added subscribers: chrib, kristof.beyls. · View Herald Transcript
efriedma added inline comments.May 11 2018, 4:52 PM
lib/Target/ARM/ARMFrameLowering.cpp
1624 ↗(On Diff #146416)

Where does this StackEstimate come from? Could it be factored out into a helper?

mstorsjo added inline comments.May 11 2018, 11:39 PM
lib/Target/ARM/ARMFrameLowering.cpp
1624 ↗(On Diff #146416)

Hmm, unsure how feasible it is... But after looking closer, it seems we don't really need all of it; MFI.estimateStackSize(MF) should in itself be the value needed (or an overestimate of it).

mstorsjo updated this revision to Diff 146463.May 11 2018, 11:40 PM

Simplified the estimate, elaborated the comment a little.

javed.absar added inline comments.May 12 2018, 9:06 AM
lib/Target/ARM/ARMFrameLowering.cpp
1630 ↗(On Diff #146463)

The original comments above (1614-1618) mention R4 but we are dealing with LR as well here now. Would it be better to separate the two?

mstorsjo added inline comments.May 12 2018, 10:09 AM
lib/Target/ARM/ARMFrameLowering.cpp
1630 ↗(On Diff #146463)

Sure, I can do that.

mstorsjo updated this revision to Diff 146473.May 12 2018, 10:10 AM

Separated the blocks, as requested.

javed.absar added a comment.May 14 2018, 1:37 AM
In D46777#1096889, @mstorsjo wrote:

Separated the blocks, as requested.

I think you forgot to upload updated patch.

mstorsjo updated this revision to Diff 146551.May 14 2018, 1:50 AM

Actually updated the patch

mstorsjo added a comment.May 14 2018, 1:50 AM
In D46777#1097475, @javed.absar wrote:
In D46777#1096889, @mstorsjo wrote:

Separated the blocks, as requested.

I think you forgot to upload updated patch.

Oops, sorry about that. Updated for real now.

javed.absar accepted this revision.May 14 2018, 2:00 AM
This revision is now accepted and ready to land.May 14 2018, 2:00 AM
Closed by commit rL332298: [ARM] Back up R4 and LR if calling the stack probe function (authored by mstorsjo). · Explain WhyMay 14 2018, 2:36 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Target/
ARM/
11 lines
test/
CodeGen/
ARM/
Windows/
2 lines

Diff 146691

llvm/trunk/lib/Target/ARM/ARMFrameLowering.cpp

Show First 20 Lines • Show All 1,614 Lines • ▼ Show 20 Linesvoid ARMFrameLowering::determineCalleeSaves(MachineFunction &MF,
// scratch register. Also spill R4 if Thumb2 function has varsized objects, // scratch register. Also spill R4 if Thumb2 function has varsized objects,
// since it's not always possible to restore sp from fp in a single // since it's not always possible to restore sp from fp in a single
// instruction. // instruction.
// FIXME: It will be better just to find spare register here. // FIXME: It will be better just to find spare register here.
if (AFI->isThumb2Function() && if (AFI->isThumb2Function() &&
(MFI.hasVarSizedObjects() || RegInfo->needsStackRealignment(MF))) (MFI.hasVarSizedObjects() || RegInfo->needsStackRealignment(MF)))
SavedRegs.set(ARM::R4); SavedRegs.set(ARM::R4);
// If a stack probe will be emitted, spill R4 and LR, since they are
// clobbered by the stack probe call.
// This estimate should be a safe, conservative estimate. The actual
// stack probe is enabled based on the size of the local objects;
// this estimate also includes the varargs store size.
if (STI.isTargetWindows() &&
WindowsRequiresStackProbe(MF, MFI.estimateStackSize(MF))) {
SavedRegs.set(ARM::R4);
SavedRegs.set(ARM::LR);
}
if (AFI->isThumb1OnlyFunction()) { if (AFI->isThumb1OnlyFunction()) {
// Spill LR if Thumb1 function uses variable length argument lists. // Spill LR if Thumb1 function uses variable length argument lists.
if (AFI->getArgRegsSaveSize() > 0) if (AFI->getArgRegsSaveSize() > 0)
SavedRegs.set(ARM::LR); SavedRegs.set(ARM::LR);
// Spill R4 if Thumb1 epilogue has to restore SP from FP or the function // Spill R4 if Thumb1 epilogue has to restore SP from FP or the function
// requires stack alignment. We don't know for sure what the stack size // requires stack alignment. We don't know for sure what the stack size
// will be, but for this, an estimate is good enough. If there anything // will be, but for this, an estimate is good enough. If there anything
▲ Show 20 Lines • Show All 857 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/ARM/Windows/chkstk-movw-movt-isel.ll

Show All 12 Linesentry:
%0 = load i32, i32* %i.addr, align 4 %0 = load i32, i32* %i.addr, align 4
%rem = urem i32 %0, 4096 %rem = urem i32 %0, 4096
%arrayidx = getelementptr inbounds [4096 x i8], [4096 x i8]* %buffer, i32 0, i32 %rem %arrayidx = getelementptr inbounds [4096 x i8], [4096 x i8]* %buffer, i32 0, i32 %rem
%1 = load volatile i8, i8* %arrayidx, align 1 %1 = load volatile i8, i8* %arrayidx, align 1
ret i8 %1 ret i8 %1
} }
; CHECK-LABEL: isel ; CHECK-LABEL: isel
; CHECK: push {r4, r5} ; CHECK: push {r4, r5, r6, lr}
; CHECK: movw r12, #0 ; CHECK: movw r12, #0
; CHECK: movt r12, #0 ; CHECK: movt r12, #0
; CHECK: movw r4, #{{\d*}} ; CHECK: movw r4, #{{\d*}}
; CHECK: blx r12 ; CHECK: blx r12
; CHECK: sub.w sp, sp, r4 ; CHECK: sub.w sp, sp, r4