This is an archive of the discontinued LLVM Phabricator instance.

[ppc64] Enable sibling call optimization on ppc64 ELFv1/ELFv2 abi
ClosedPublic

Authored by cycheng on Jan 19 2016, 6:08 AM.

Details

Reviewers
kbarton
hfinkel
Summary

This patch enable sibling call optimization on ppc64 ELFv1/ELFv2 abi, and add a couple of test cases.
This patch also passed llvm/clang bootstrap test, and spec2006 build/run/result validation.

Original issue: https://llvm.org/bugs/show_bug.cgi?id=25617

Great thanks to Tom's help, he contributed a lot to this patch.

Diff Detail

Event Timeline

cycheng updated this revision to Diff 45245.Jan 19 2016, 6:08 AM
cycheng retitled this revision from to [ppc64] Enable sibling call optimization on ppc64 ELFv1/ELFv2 abi.
cycheng updated this object.
cycheng added reviewers: hfinkel, kbarton.
cycheng added subscribers: tjablin, llvm-commits.
hfinkel added inline comments.Feb 3 2016, 5:53 AM
lib/Target/PowerPC/PPCISelLowering.cpp
3858

Function names should start with a lower-case letter.

3898

Function names should start with a lower-case letter.

3929

Indent more to line up with other args.

3963

Use std::any_of (with a small lambda).

3986

Are you planning on doing this in follow-up? It seems simple enough to check. Unless there's some complication here, I'd rather you do this up-front so that we get more testing coverage on all of the rest of the logic.

4017

Variables start with a capital letter.

4020

Variables start with a capital letter.

4020

I don't think that the first live-in register in the list is guaranteed to be the first argument's register. Also, you say "vreg id"; are these virtual registers or physical registers at this point?

cycheng added inline comments.Feb 5 2016, 7:45 AM
lib/Target/PowerPC/PPCISelLowering.cpp
3986

Then I can generate second patch quickly ; P

OK, I am going to fix it in this patch.

4020

Mmmm.. I will try to find other method for getting first argument.

They are virtual registers.

cycheng updated this revision to Diff 48780.Feb 23 2016, 7:39 AM
cycheng marked 6 inline comments as done.

Fix all issues mentioned in http://reviews.llvm.org/D16315#342919

  • [OK] Use std::any_of
  • [OK] Source Format
  • [OK] Implement Caller/Callee Argument Checking, If Callee and Caller have the same arguments, we can apply SCO on it.
  • [OK] Delete struct return checking code because llvm has guaranteed it.

This patch passed llvm bootstrap test, we also used the bootstrap result to build/run/validate SPEC2006 INT benchmark, and it passed SPEC2006 INT test. The llvm version we use is "r251575" (Oct 28 2015)

But we found the latest revision of llvm (r257864) with our patch can crash in bootstrap test stage 3, we are investigating it now.

About struct return: Originally, we worried if Callee and Caller can use different struct return pointer. But llvm has check it for us (SelectionDAGBuilder::LowerCallTo), so we delete the code.

cycheng updated this revision to Diff 48922.Feb 24 2016, 6:29 AM

Undo unnecessary changes.

We found a bug when we enable shrink-wrapping + SCO, and do llvm bootstrap testing.
After we dig into the bug, we think it's NOT SCO's issue, we have tested disable shrink-wrapping, this patch can pass bootstrap testing, spec2006 testing.

The bug is, in some cases, shrink-wrapping + SCO will cause the tail call branch disappear, we don't know why and we are investigating it.

cycheng updated this revision to Diff 49053.EditedFeb 25 2016, 6:50 AM

Add new test case for testing combination of sibling call optimizing and shrink wrapping.

NOTE! This patch is dependent on http://reviews.llvm.org/D17606

kbarton added inline comments.Mar 30 2016, 8:38 AM
lib/Target/PowerPC/PPCISelLowering.cpp
3916

Variable names should be camel case (i.e., remove the underscores).
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

3936

I'd recommend const Function *CallerFn, instead of const Function* CallerFn.
The * modifies the parameter, not the type, so it makes sense to put it closer to the parameter.

3973

The GuaranteedTailCallOpt concerns me. Is this used to guarantee functional correctness in some cases?
There are several checks below that will return false, even if TallCallOpt is true. Are we guaranteed this is always going to be correct?

If it is not for correctness, then why can it not be disabled? It overrides the DisableSCO flag on line 3971, but then is ignored later on line 4008.

4675

Please add a message here indicating why this assert fails.

test/CodeGen/PowerPC/ppc64-sibcall-shrinkwrap.ll
3

Please add run steps for powerpc64le-unknown-linux-gnu as well.

test/CodeGen/PowerPC/ppc64-sibcall.ll
3

Please add run steps for powerpc64le-unknown-linux-gnu as well.

cycheng updated this revision to Diff 52344.Apr 1 2016, 3:49 AM
cycheng marked 5 inline comments as done.

Fixed issues mentioned by Kit

lib/Target/PowerPC/PPCISelLowering.cpp
3916

Thanks!

3936

Agree!

3973

"GuaranteedTailCallOpt" is original code path.

SibCallOpt and TailCallOpt share the following condition checking:

  • isVarArg
  • Same Calling Convention
  • by val parameters checking
  • indirect call checking
  • resideInSameModule checking

Specific for TailCallOpt checking:

  • Calling convention should be "CallingConv::Fast"

Specific for SibCallOpt checking:

  • hasSameArgumentList
  • needStackSlotPassParameters

So even if TallCallOpt is true, there are still some cases that can not be TailCallOptimized.
And if TallCallOpt is true, we don't need to check SibCallOpt condition.

4675
assert(isa<GlobalAddressSDNode>(Callee) &&
       "Callee should be an llvm::Function object.");
kbarton accepted this revision.Apr 1 2016, 1:09 PM
kbarton edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 1 2016, 1:09 PM
cycheng closed this revision.Apr 5 2016, 7:10 PM

Committed r265506