This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support stack frames and offsets up to 32-bits
ClosedPublic

Authored by asb on Dec 4 2017, 2:32 PM.

Details

Summary

Also, factor out stack pointer manipulation to use the new 'adjustReg' helper function.

I think this is fairly straight-forward, but I'd appreciate an extra pair of eyes just to recheck I'm setting killflags etc appropriately.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Dec 4 2017, 2:32 PM
reames requested changes to this revision.Dec 4 2017, 7:03 PM
reames added a subscriber: reames.

There are a couple of NFC parts that need extracted here before the semantic change can be easily reviewed.

lib/Target/RISCV/RISCVFrameLowering.cpp
60 ↗(On Diff #125420)

Looks like this is an RFC extraction. Feel free to separate and land.

71 ↗(On Diff #125420)

You could add a case for when Val == 0 and use a reg move.

Hm, reviewing the ISA doc, it doesn't look like there's a register register move? Ok, add is fine. :)

84 ↗(On Diff #125420)

This really looks like a helper function along the lines of materializedInt32(Reg, Val). I'm pretty sure you already have other copies of this code as well.

91 ↗(On Diff #125420)

Is it correct to indicate a kill on a use when we're updating the same scratch reg?

lib/Target/RISCV/RISCVRegisterInfo.cpp
85 ↗(On Diff #125420)

Two problems:

  1. You're missing an assert that this is an int32.
  2. This is the other use of materializeInt32 I mentioned above. :)
This revision now requires changes to proceed.Dec 4 2017, 7:03 PM
asb updated this revision to Diff 126296.Dec 10 2017, 9:24 AM
asb edited edge metadata.
asb marked 5 inline comments as done.

Update to address review comments. adjustReg is now introduced in an earlier patch, and we also add RISCVInstrInfo::movImm32 in order to reduce duplication of logic.

lib/Target/RISCV/RISCVFrameLowering.cpp
71 ↗(On Diff #125420)

Correct, there is an assembler pseudoinstruction but mv rd, rs is just addi rd, rs, 0.

91 ↗(On Diff #125420)

I believe so, or at least there seem to be a number of examples of the same thing within lib/Target/*.

reames accepted this revision.Dec 13 2017, 7:10 PM

Much cleaner, thanks!

This revision is now accepted and ready to land.Dec 13 2017, 7:10 PM
asb updated this revision to Diff 127102.Dec 15 2017, 4:41 AM

Rebase patch.

This revision was automatically updated to reflect the committed changes.