This is an archive of the discontinued LLVM Phabricator instance.

NFC; refactor getFrameIndexReferenceFromSP
ClosedPublic

Authored by sanjoy on Jun 15 2016, 11:11 PM.

Details

Summary

... into getFrameIndexReferencePreferSP. This change folds the
fail-then-retry logic into getFrameIndexReferencePreferSP.

There is a non-functional but behaviorial change in WinException --
earlier if getFrameIndexReferenceFromSP failed we'd trip an assert,
but now we'll silently use the (wrong) offset from the base pointer. I
could not write the assert I'd like to write ("FrameReg ==
StackRegister", like I've done in X86FrameLowering) since there is no
easy way to get to the stack register from WinException (happy to be
proven wrong here). One solution to this is to add a `bool
OnlyStackPointer` parameter to getFrameIndexReferenceFromSP that
asserts if it could not satisfy its promise of returning an offset from
a stack pointer, but that seems overkill.

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 60948.Jun 15 2016, 11:11 PM
sanjoy retitled this revision from to NFC; refactor getFrameIndexReferenceFromSP.
sanjoy updated this object.
sanjoy added a reviewer: rnk.
sanjoy added a subscriber: llvm-commits.
rnk accepted this revision.Jun 16 2016, 8:20 AM
rnk edited edge metadata.

lgtm with the assert

include/llvm/Target/TargetFrameLowering.h
257

Can we rename "AllowSPAdjustment" to something better? As the doc comments say, it's really about getting the offset relative to SP at end of prologue. A better name might be IgnoreSPUpdates or IgnoreSPAdjustments or OffsetFromInitialSP or OffsetFromSPAfterPrologue.

lib/CodeGen/AsmPrinter/WinException.cpp
307

We could try this assert:

assert(UnusedReg == Asm->MF->getSubtarget().getTargetLowering()->getStackPointerRegisterToSaveRestore());

Beautiful, I know.

The only way this can end up using the FP and we fail the assert is if we try to reference a fixed object with stack realignment. In this code, FrameIndex should never refer to a fixed stack object, so we should be OK.

This revision is now accepted and ready to land.Jun 16 2016, 8:20 AM
This revision was automatically updated to reflect the committed changes.