Page MenuHomePhabricator

[PPC] Fix two bugs in frame lowering.
ClosedPublic

Authored by jtony on Jun 18 2017, 6:53 PM.

Details

Summary

(1) The available program storage region of the red zone to compilers is 288 bytes rather than 244 bytes (both for ABI V1.9 and ABI V2).
(2) The formula for negative number alignment calculation should be y = x & ~(n-1) rather than y = (x + (n-1)) & ~(n-1).

Diff Detail

Repository
rL LLVM

Event Timeline

jtony created this revision.Jun 18 2017, 6:53 PM
inouehrs added inline comments.Jun 18 2017, 11:47 PM
test/CodeGen/PowerPC/tailcall1-64.ll
2 ↗(On Diff #102983)

Does this change relate to frame lowering?

jtony edited the summary of this revision. (Show Details)Jun 19 2017, 6:21 AM
jtony edited the summary of this revision. (Show Details)
jtony edited the summary of this revision. (Show Details)
jtony added inline comments.Jun 19 2017, 8:59 AM
test/CodeGen/PowerPC/tailcall1-64.ll
2 ↗(On Diff #102983)

This test case is modified to make sure the changes above don't regress the TCO.

nemanjai added inline comments.Jun 19 2017, 11:10 PM
lib/Target/PowerPC/PPCFrameLowering.cpp
1877 ↗(On Diff #102983)

How are we testing this change?

stefanp added inline comments.Jun 20 2017, 1:11 PM
lib/Target/PowerPC/PPCFrameLowering.cpp
1874 ↗(On Diff #102983)

nit: (the stack is growning down) -> (the stack grows downward)

test/CodeGen/PowerPC/tailcall1-64.ll
12 ↗(On Diff #102983)

nit: Should we be doing a CHECK-LABEL first?

jtony updated this revision to Diff 103368.Jun 21 2017, 6:50 AM
jtony marked 5 inline comments as done.

Address the comments from Nemanja and Stefan.
Add one test case to test the alignment calculation change.

jtony edited the summary of this revision. (Show Details)Jun 21 2017, 7:58 AM
nemanjai accepted this revision.Jun 29 2017, 12:44 AM

LGTM. If @hfinkel doesn't have any objections, I think this can go in.

This revision is now accepted and ready to land.Jun 29 2017, 12:44 AM
hfinkel edited edge metadata.Jun 29 2017, 8:03 AM

LGTM. If @hfinkel doesn't have any objections, I think this can go in.

So... I believe if we make this change as-is then we've dropped Darwin support. Darwin had a 224-byte red zone. I'm in favor of doing that, but I don't think we should just do it subtly like this. I'd add a check for Darwin for now (to keep the 224). Then we can rip it out with the rest of Darwin support when we do that.

lib/Target/PowerPC/PPCFrameLowering.cpp
1873 ↗(On Diff #103368)

Please add parens here (x + (n-1)) & (n-1).

jtony updated this revision to Diff 105493.Jul 6 2017, 11:25 AM
jtony marked an inline comment as done.

(1) Refactor the messy red zone guard conditions to make it more readable.
(2) Differentiate the red zone behavior for DarwinABI and Non-DarwinABI (SVR4ABI).
(3) Address comments from Hal.

inouehrs added inline comments.Jul 6 2017, 12:30 PM
lib/Target/PowerPC/PPCFrameLowering.cpp
457 ↗(On Diff #105493)

How about adding a function Subtarget.getAvailableRedZoneSize(), which returns 288 only when icPPC64 && isSVR4ABI and 224 otherwise?

jtony added inline comments.Jul 6 2017, 12:58 PM
lib/Target/PowerPC/PPCFrameLowering.cpp
1877 ↗(On Diff #102983)

I added one new test cases to test the alignment calculation change.

457 ↗(On Diff #105493)

I understand why you want to add that function, but if you look at the logic (!Subtarget.isPPC64()) && (FrameSize == 0) can not nicely fit into that refactor. If everything can be fit into the getAvailableRedZoneSize() function and we just use
bool FitsInRedZone = FrameSize <= Subtarget.getAvailableRedZoneSize(), the refactoring you mentioned would be more useful.

inouehrs added inline comments.Jul 6 2017, 6:26 PM
lib/Target/PowerPC/PPCFrameLowering.cpp
457 ↗(On Diff #105493)
isSVR4ABI && isPPC64 -> return 288
isSVR4ABI && !isPPC64 -> return 0
other -> return  224

Is this representing the above conditions? (since FrameSize is unsigned FrameSize == 0 and FrameSize <= 0 is same.)

jtony updated this revision to Diff 105796.Jul 9 2017, 4:01 PM
jtony marked 3 inline comments as done.

Address comments from Hiroshi.

kbarton accepted this revision.Jul 10 2017, 9:15 AM

LGTM. Just some minor updates in the comments.

lib/Target/PowerPC/PPCSubtarget.h
277 ↗(On Diff #105796)

Please change the Non-DarwinABI to SVR4ABI.

test/CodeGen/PowerPC/ppc-redzone-alignment-bug.ll
2 ↗(On Diff #105796)

Can you please replace "this issue" with the specific Review that is being fixed?

10 ↗(On Diff #105796)

Again, instead of "the fix" specify the review this is being done under.

jtony marked 3 inline comments as done.Jul 10 2017, 1:29 PM
This revision was automatically updated to reflect the committed changes.