Page MenuHomePhabricator

[PPC] Properly update register save area offsets
ClosedPublic

Authored by kparzysz on May 9 2017, 3:12 PM.

Details

Summary

The variables MinGPR/MinG8R were not updated properly when resetting the offsets, which in the included testcase lead to saving the CR register in the same location as R30.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz created this revision.May 9 2017, 3:12 PM

Mark Millard was kind enough to test this patch via buildworld on FreeBSD/ppc32. See https://bugs.llvm.org/show_bug.cgi?id=26519 for details.

nemanjai edited edge metadata.May 12 2017, 5:01 PM

Do we have anything we can/want to test on 64-bit targets considering there's a change that presumably affects them (i.e. the update for the base pointer).

In any case, save for the 64-bit test case, this looks perfectly fine to me but I think I should defer the final OK to @hfinkel as he's way more familiar with this code than I am.

lib/Target/PowerPC/PPCFrameLowering.cpp
1790 ↗(On Diff #98356)

I am frankly not familiar enough with the frame lowering code or the concept of the base pointer, but I think a comment here is in order.
I suppose that when a function has a base pointer and uses no callee-saved registers, we need a slot for the base pointer.

I don't have access to PPC hardware so there isn't much that I can do in terms of testing other than running "check-llvm".

lib/Target/PowerPC/PPCFrameLowering.cpp
1790 ↗(On Diff #98356)

The base pointer is a "stack pointer" that is aligned to a boundary that exceeds the natural stack alignment. The code added in this patch checks if it's a 32- or 64-bit register to update the correct variable.

I don't have access to PPC hardware so there isn't much that I can do in terms of testing other than running "check-llvm".

I'll test this on ppc64le and will update this patch with a comment with the results of the testing when it's done.

This passes a double bootstrap with full testing. So I think it's fine. However, are you able to add test cases for 32/64-bit targets that just saves the base pointer (i.e. a function that will use a base pointer without using any callee-saved registers and will save the base pointer - X30/R30/R29)?

hfinkel accepted this revision.May 16 2017, 11:04 AM

This passes a double bootstrap with full testing. So I think it's fine.

This looks good to me as well. Once you come up with a test as requested, or figure out that you can't, please go ahead.

However, are you able to add test cases for 32/64-bit targets that just saves the base pointer (i.e. a function that will use a base pointer without using any callee-saved registers and will save the base pointer - X30/R30/R29)?

This revision is now accepted and ready to land.May 16 2017, 11:04 AM

Many thanks to both of you!

This revision was automatically updated to reflect the committed changes.