This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Do not grow the stack a second time when we need to realign the stack
ClosedPublic

Authored by rogfer01 on Oct 12 2020, 7:09 AM.

Details

Summary

This is a first change needed to fix a crash in which the emergency spill splot ends being out of reach. This happens when we run the register scavenger after we have eliminated the frame indexes. The fix for the actual crash will come in a later change.

This change removes an extra stack size increase we do in RISCVFrameLowering::determineFrameLayout.

My understanding is that we don't have to change the size of the stack here as PEI::calculateFrameObjectOffsets is already doing this (see https://github.com/llvm/llvm-project/blob/539381da26096df54ccf862088c8242498a7dcae/llvm/lib/CodeGen/PrologEpilogInserter.cpp#L1085 ) with the right size accounting the extra alignment as kept in MaxAlign (see https://github.com/llvm/llvm-project/blob/539381da26096df54ccf862088c8242498a7dcae/llvm/lib/CodeGen/PrologEpilogInserter.cpp#L1079 ).

Diff Detail

Event Timeline

rogfer01 created this revision.Oct 12 2020, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2020, 7:09 AM
rogfer01 requested review of this revision.Oct 12 2020, 7:09 AM

I thought we were over-allocating stack frame sizes, but I hadn't had time to investigate this properly, thanks for the thorough investigation Roger!

(There's a comment about some of the same issues in one of the other backends, but I cannot find it, I can find my incorrect fix though: D65959, since abandoned).

luismarques accepted this revision.Oct 12 2020, 8:25 AM

LGTM. Thanks for the detailed description, with the links.

This revision is now accepted and ready to land.Oct 12 2020, 8:25 AM
asb accepted this revision.Oct 15 2020, 5:04 AM

Thanks Roger, this looks good to me and doesn't seem to break anything in the GCC torture suite at least.

HsiangKai accepted this revision.Jan 5 2021, 11:30 PM
HsiangKai added a subscriber: HsiangKai.

LGTM.

rogfer01 updated this revision to Diff 315606.Jan 9 2021, 8:36 AM

ChangeLog:

  • Rebase patch prior committing