Page MenuHomePhabricator

[RISCV] Minimal stack realignment support
ClosedPublic

Authored by lenary on May 16 2019, 6:48 AM.

Details

Summary

Currently the RISC-V backend does not realign the stack. This can be an issue even for the RV32I/RV64I ABIs (where the stack is 16-byte aligned), though is rare. It will be much more comment with RV32E (though the alignment requirements for common data types remain under-documented...).

This patch adds minimal support for stack realignment. It should cope with large realignments. It will error out if the stack needs realignment and variable sized objects are present.

It feels like a lot of the code like getFrameIndexReference and determineFrameLayout could be refactored somehow, as right now it feels fiddly and brittle. We also seem to allocate a lot more memory than GCC does for equivalent C code.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.May 16 2019, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 6:48 AM
lenary added a subscriber: lenary.Tue, Jul 23, 5:27 AM
lenary commandeered this revision.Thu, Jul 25, 5:10 AM
lenary added a reviewer: asb.

I'm taking this off @asb's plate, because we want it to be backported to the LLVM 9.0 branch, and I have more time to investigate the issue than he does right now.

lenary updated this revision to Diff 211724.Thu, Jul 25, 5:24 AM

Rebased against master

lenary updated this revision to Diff 212318.Tue, Jul 30, 5:34 AM

Working on Stack Realignment

  • Support massive realignments (hopefully not needed, but one less ICE)
  • Make the tests more comprehensive

We're still allocating way more than GCC does for stack frames. I'm not sure if
this is an LLVM problem or a RISC-V backend problem, any ideas @asb?

lenary retitled this revision from [RISCV][WIP] Minimal stack realignment support to [RISCV] Minimal stack realignment support.Tue, Jul 30, 8:09 AM
lenary edited the summary of this revision. (Show Details)
wwei added a subscriber: wwei.Wed, Jul 31, 7:51 AM

@lenary @asb It seems that PEI::calculateFrameObjectOffsets function in PrologEpilogInserter pass has done some stack size rounding up work. Each stack object will call AdjustStackOffset to adjust the stack frame offset, and corresponding update may be made for MaxAlign.
Also, there's a TFI hook targetHandlesStackFrameRounding to decide if the target is responsible for rounding up the stack frame(default return false). For RISCV, maybe this function should return true I think, thus the stack size for StackRealignment will be calculated at RISCV's emitPrologue time.
For the implementation in this patch, the stack size for StackRealignment will be calculathed two times, one in PEI::calculateFrameObjectOffsets, another time in RISCVFrameLowering::determineFrameLayout, and the stack size may become bigger.)

lenary updated this revision to Diff 213361.Mon, Aug 5, 7:26 AM
lenary edited the summary of this revision. (Show Details)

Address review feedback

  • implement targetHandlesStackFrameRounding to prevent stack being rounded twice.
asb added a comment.Tue, Aug 6, 7:31 AM

Address review feedback

  • implement targetHandlesStackFrameRounding to prevent stack being rounded twice.

Could you please implement this hook in a separate patch (either a parent or child of this one). The stack allocation and prolog/epilog code is a fiddly part of the backend, where there have been bugs in the past and I'm sure there will be bugs in the future. I'd much rather separate the changes to allow easily inspection, bisection etc.

lenary updated this revision to Diff 213862.Wed, Aug 7, 6:17 AM

Undo implementation of targetHandlesStackFrameRounding

asb accepted this revision.Thu, Aug 8, 4:13 AM

LGTM, thanks!

Please to submit a follow-on patch for targetHandlesStackFrameRounding

Is there a reason why you want tests cases for all 32/64/128/256/512/1024/2048 alignment? It feels like 32, 2048, and 4096 would give sufficient coverage?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
301 ↗(On Diff #213862)

still stack => still access stack

This revision is now accepted and ready to land.Thu, Aug 8, 4:13 AM
asb added a comment.Thu, Aug 8, 6:47 AM

Also, clang-format seems to prefer to format some of this patch differently - worth running it through again.

lenary updated this revision to Diff 214151.Thu, Aug 8, 7:37 AM

Address review feedback

  • Format changes
  • Reword comment
lenary marked an inline comment as done.Thu, Aug 8, 7:37 AM
This revision was automatically updated to reflect the committed changes.
lenary added a subscriber: hans.Tue, Aug 13, 10:07 AM

@hans Please may you backport this to the 9.0 branch?

hans added a comment.Wed, Aug 14, 5:58 AM

@hans Please may you backport this to the 9.0 branch?

Sure, merged in r368846.