This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Break the dependency between FP and SP when possible.
ClosedPublic

Authored by hiraditya on Mar 10 2016, 11:57 AM.

Details

Summary

In collaboration with Evandro Menezes.

When the SP in not changed because of realignment/VLAs etc., we restore the SP by using the previous value of SP and not the FP. Breaking the dependency will help in cases when
the epilog of a callee is close to the epilog of the caller; for then "sub sp, fp, #" depends on the load restoring the FP in the epilog of the callee.

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya updated this revision to Diff 50328.Mar 10 2016, 11:57 AM
hiraditya retitled this revision from to [AArch64] Break the dependency between FP and SP when possible..
hiraditya updated this object.
hiraditya added reviewers: mcrosier, t.p.northover.
hiraditya set the repository for this revision to rL LLVM.
hiraditya added a subscriber: evandro.
mcrosier resigned from this revision.Mar 10 2016, 12:00 PM
mcrosier edited reviewers, added: gberry; removed: mcrosier.
mcrosier added inline comments.
lib/Target/AArch64/AArch64FrameLowering.h
22 ↗(On Diff #50328)

stackRealigned should be capitalized.

hiraditya updated this revision to Diff 50334.Mar 10 2016, 12:42 PM

Capitalized the member variable.

hiraditya marked an inline comment as done.Mar 10 2016, 12:42 PM
t.p.northover edited edge metadata.Mar 10 2016, 1:06 PM

Hi,

This looks like a big hammer for a really weird corner case. How often is a write to fp close enough to the epilogue for this to matter? If it really is a frame pointer in that function, it'll be last written in the prologue; and if it's not the frame pointer it can't be liveout so aren't you likely to have some credible use of fp between the def and the epilogue anyway?

Cheers.

Tim.

lib/Target/AArch64/AArch64FrameLowering.h
22 ↗(On Diff #50334)

This should be in AArch64MachineFunctionInfo.h too. Adding the very first mutable variable to an otherwise completely empty class should really have been a warning flag.

This looks like a big hammer for a really weird corner case. How often is a write to fp close enough to the epilogue for this to matter?

It's not when the prolog is close to the epilog, but when the epilog of a callee is close to the epilog of the caller, for then "sub sp, fp, #" depends on the load restoring the FP in the epilog of the callee, which is typically on the long side, even if it does not miss.

Ah, that makes more sense. Thanks.

hiraditya added inline comments.Mar 11 2016, 8:09 AM
lib/Target/AArch64/AArch64FrameLowering.h
22 ↗(On Diff #50334)

Thanks for the feedback. I'll try to find more appropriate place to put this variable.

gberry edited edge metadata.Mar 11 2016, 8:36 AM

Could you add a bit more description to the commit message along the lines of your response to Tim's comment about the motivation for this change?

lib/Target/AArch64/AArch64FrameLowering.cpp
612

Minor nit: you might want to swap the order of these so you do the MFI load conditionally.

hiraditya updated this revision to Diff 50440.Mar 11 2016, 9:06 AM
hiraditya edited edge metadata.

Moved the variable StackRealigned to AArch64MachineFunctionInfo.h

hiraditya updated this object.Mar 11 2016, 9:10 AM
t.p.northover added inline comments.Mar 11 2016, 9:15 AM
lib/Target/AArch64/AArch64FrameLowering.cpp
298

Isn't this redundant now that it's in per-function storage?

hiraditya updated this revision to Diff 50442.Mar 11 2016, 9:48 AM

Removed redundant assignment.

hiraditya marked 2 inline comments as done.Mar 11 2016, 9:49 AM
t.p.northover accepted this revision.Mar 11 2016, 4:02 PM
t.p.northover edited edge metadata.

I think this is good now. Thanks for the updates.

Tim.

This revision is now accepted and ready to land.Mar 11 2016, 4:02 PM

Hi Tim,
Thanks for accepting the patch. Could you please commit this patch as I do not have commit approval.

Thanks,
-Aditya

mcrosier closed this revision.Mar 14 2016, 11:23 AM

Committed r263458.