This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement prolog and epilog insertion
ClosedPublic

Authored by asb on Nov 9 2017, 10:02 AM.

Details

Summary

As frame pointer elimination isn't implemented until a later patch and we make extensive use of update_llc_test_checks.py, this changes touches a lot of the RISC-V tests.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Nov 9 2017, 10:02 AM
mgrang added a subscriber: mgrang.Nov 9 2017, 10:24 AM
mgrang added inline comments.
lib/Target/RISCV/RISCVFrameLowering.cpp
94 ↗(On Diff #122262)

nit: Period after comment.

111 ↗(On Diff #122262)

nit: Period after comment.

167 ↗(On Diff #122262)

nit: Period after comment.

lib/Target/RISCV/RISCVRegisterInfo.cpp
76 ↗(On Diff #122262)

nit: Period after comment.

asb updated this revision to Diff 122297.Nov 9 2017, 11:51 AM
asb marked 4 inline comments as done.
asb edited the summary of this revision. (Show Details)

Address @mgrang's review comments (thanks!). I also moved the changes to eliminateFrameIndex to the previous patch, as it makes sense to just do it right from the start.

sameer.abuasal edited reviewers, added: sabuasal; removed: sameer.abuasal.Nov 13 2017, 11:49 AM
glasnak added inline comments.
lib/Target/RISCV/RISCVFrameLowering.cpp
31 ↗(On Diff #122297)

shouldn't this be uint64_t?

(also not sure about MaxCallFrameSize below, as alignTo() returns uint64_t. It gets two unsigned ints, so I'll leave that up to you if it's necessary to change just to satisfy linters)

170 ↗(On Diff #122297)

redundant return?

asb updated this revision to Diff 123762.Nov 21 2017, 4:19 AM
asb marked 2 inline comments as done.

Rebase and address review comments.

mgrang accepted this revision.Nov 21 2017, 10:57 AM

LGTM.

This revision is now accepted and ready to land.Nov 21 2017, 10:57 AM
sabuasal added inline comments.Nov 21 2017, 8:57 PM
lib/Target/RISCV/RISCVFrameLowering.cpp
90 ↗(On Diff #123762)

How should we handle cases where we need a bigger stack?

asb updated this revision to Diff 125375.Dec 4 2017, 10:55 AM

Refreshing the patch. Just awaiting the dependent frameindex patch to be reviewed in order to merge.

reames added a subscriber: reames.Dec 4 2017, 7:08 PM

small drive by comments.

lib/Target/RISCV/RISCVFrameLowering.cpp
130 ↗(On Diff #125375)

You've got these in a couple of places, time for an alias in the enum?

test/CodeGen/RISCV/addc-adde-sube-subc.ll
10 ↗(On Diff #125375)

The test churn in this patch is really unfortunate. Is there a small change you can add to not need all the spill code for most functions?

asb updated this revision to Diff 126142.Dec 8 2017, 7:01 AM
asb marked an inline comment as done.

Rebase patch, and address minor review comments.

asb updated this revision to Diff 126295.Dec 10 2017, 9:18 AM

Rebase and use an adjustReg helper (this minimises the diff for D40807).

lib/Target/RISCV/RISCVFrameLowering.cpp
130 ↗(On Diff #125375)

I've added some helper functions to return FPReg and SPReg. They take RISCVSubtargetInfo to allow future extensibility (e.g. different ABIs).

test/CodeGen/RISCV/addc-adde-sube-subc.ll
10 ↗(On Diff #125375)

Frame pointer elimination isn't a particularly large change (see here). Although verbose generated code has disadvantages, I think there are benefits to "completing" codegen before supporting extra optimisations, even fairly trivial ones. Although ultimately a straight-forward change, a large percentage of the bugs I've encountered while developing the backend have actually been related to frame handling (whether as part of argument passing or otherwise).

This revision was automatically updated to reflect the committed changes.