This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement prolog and epilog insertion

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



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

Diff Detail


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.
94 ↗(On Diff #122262)

nit: Period after comment.

111 ↗(On Diff #122262)

nit: Period after comment.

167 ↗(On Diff #122262)

nit: Period after comment.

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.
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


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
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.

130 ↗(On Diff #125375)

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

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).

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).

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.