This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Support huge frame size for PPC64
ClosedPublic

Authored by lkail on Aug 11 2021, 12:38 AM.

Details

Reviewers
nemanjai
shchenz
jsji
Group Reviewers
Restricted Project
Commits
rG5018a5dcbe70: [PowerPC] Support huge frame size for PPC64
Summary

Support allocation of huge stack frame(>2g) on PPC64.

For ELFv2 ABI on Linux, quoted from the spec 2.2.3.1 General Stack Frame Requirements

There is no maximum stack frame size defined.

On AIX, XL allows such huge frame.

Diff Detail

Event Timeline

lkail created this revision.Aug 11 2021, 12:38 AM
lkail requested review of this revision.Aug 11 2021, 12:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2021, 12:38 AM
nemanjai requested changes to this revision.Aug 11 2021, 5:58 AM

Do we have a practical requirement for such a thing? At the very least, I believe we will need code in FI elimination to handle these absurd frame sizes and the test case should show accesses to the stack both before and after the huge allocation.

This revision now requires changes to proceed.Aug 11 2021, 5:58 AM
lkail updated this revision to Diff 365947.Aug 12 2021, 2:41 AM

Update to eliminate frameindex in huge frame.

lkail planned changes to this revision.Aug 12 2021, 2:48 AM

I'm still testing the patch with variant workloads to insure corner cases are handled correctly.

lkail requested review of this revision.Aug 31 2021, 6:52 PM

Tested it with test-suite and other benchmarks on AIX and Linux, no regression found.

lkail retitled this revision from [PowerPC][2/n] Support huge frame size for PPC64 to [PowerPC] Support huge frame size for PPC64.Aug 31 2021, 7:31 PM
lkail updated this revision to Diff 371273.Sep 8 2021, 1:29 AM

Move materialization into a routine.

nemanjai added inline comments.Oct 26 2021, 4:55 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
629

You remove this here but this pre-condition still holds for 32-bit mode. Why not change the condition to only trap in 32-bit mode rather than never?

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3242

"Receiver" sounds strange. "Target" seems like a more common name.

3250

I am not suggesting we need to re-implement optimal constant materialization here, but it seems trivially easy to only emit the ORI variants only if the immediate is non-zero.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
1553

Why is this repeated here rather than calling materializeImmPostRA() as you do in other places?

lkail updated this revision to Diff 382567.Oct 27 2021, 2:28 AM

Address comments.

lkail marked 2 inline comments as done.Oct 27 2021, 2:35 AM
lkail added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
1553

This is subtle, I've checked the commit https://github.com/llvm/llvm-project/commit/d8a423cd71bb2a9b7666fd58fc82db16b4666d1d of the code above. It intends to keep variables for Registers SSA-like but materializeImmPostRA violates this rule.

lkail added a comment.Jan 4 2022, 6:29 PM

Gentle ping.

Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 9:02 AM
lkail updated this revision to Diff 423051.Apr 15 2022, 2:13 AM

Updated affected tests.

Gentle ping.

Gentle ping.

shchenz added inline comments.May 16 2022, 1:42 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3244

Can you add the expected pattern to the FIXME for the not optimal pattern?

llvm/test/CodeGen/PowerPC/huge-frame-call.ll
24

Maybe we can avoid these cfi related instructions by adding nounwind to the function. Call frame is not a test point for this patch, right?

lkail updated this revision to Diff 431280.May 22 2022, 7:36 PM

Address comments.

This LGTM.

Let's wait for @nemanjai 's comments/approval since he requested change to this patch.

jsji resigned from this revision.Jun 2 2022, 7:52 AM
nemanjai accepted this revision.Jun 2 2022, 7:36 PM

LGTM.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3245

I think this is not a particularly great example since I imagine this would satisfy the isInt<16>() test (i.e. it looks like this is just -16).

This revision is now accepted and ready to land.Jun 2 2022, 7:36 PM
lkail updated this revision to Diff 434390.Jun 5 2022, 10:23 PM
lkail marked an inline comment as done.
This revision was landed with ongoing or failed builds.Jun 6 2022, 2:08 AM
This revision was automatically updated to reflect the committed changes.