This is an archive of the discontinued LLVM Phabricator instance.

[PPC] Set SP after loading data from stack frame, if no red zone is present
ClosedPublic

Authored by kparzysz on Sep 12 2016, 11:20 AM.

Details

Summary

Follow-up to r280705 (D24093): Make sure that the SP is only restored after all data is loaded from the stack frame, if there is no red zone. The previous patch did not address this.

This completes the fix for https://llvm.org/bugs/show_bug.cgi?id=26519.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 71036.Sep 12 2016, 11:20 AM
kparzysz retitled this revision from to [PPC] Set SP after loading data from stack frame, if no red zone is present.
kparzysz updated this object.
kparzysz added a reviewer: hfinkel.
kparzysz set the repository for this revision to rL LLVM.
kparzysz added a subscriber: llvm-commits.
hfinkel accepted this revision.Sep 12 2016, 12:33 PM
hfinkel edited edge metadata.

This LGTM, with two minor things:

  1. The form of the copies (see below)
  2. Our test coverage here seems a bit sparse. Can you please add a few more test cases to cover the (large frame vs small frame) x (with frame pointer vs. without frame pointer) x (with base pointer vs. without base pointer) configurations?
lib/Target/PowerPC/PPCFrameLowering.cpp
1348 ↗(On Diff #71036)

The code in PPCInstrInfo::copyPhysReg copies, canonically, by or'ing the value with itself. I'd prefer you use that, as the canonical representation, here.

1435 ↗(On Diff #71036)

Same comment here about the copy.

This revision is now accepted and ready to land.Sep 12 2016, 12:33 PM

Just a quick update: I'm trying to test this on PPC, but I ran into some issues bootstrapping the compiler. I hope to have this done within a few days.

kparzysz updated this revision to Diff 72066.Sep 21 2016, 9:02 AM
kparzysz edited edge metadata.

The previous code was using R0 as a base pointer in the prologue. Fixed that and added a testcase.

I have run a few tests on a PPC box using this updated patch, there were no problems.

kparzysz marked 2 inline comments as done.Sep 21 2016, 9:56 AM
kparzysz added inline comments.
test/CodeGen/PowerPC/stack-realign.ll
86 ↗(On Diff #72066)

R0 used as a base reg here, and in several cases below.

hfinkel added inline comments.Sep 22 2016, 12:21 AM
lib/Target/PowerPC/PPCFrameLowering.cpp
1017 ↗(On Diff #72066)

We have a PPC::ZERO pseudo-register for this purpose, please use that instead.

kparzysz updated this revision to Diff 72150.Sep 22 2016, 5:00 AM
kparzysz marked an inline comment as done.

LGTM

lib/Target/PowerPC/PPCFrameLowering.cpp
1039 ↗(On Diff #72150)

Don't need the "will be treated as 0" comment here.

This revision was automatically updated to reflect the committed changes.