This is an archive of the discontinued LLVM Phabricator instance.

X86: Use a callee save register for the swiftself parameter.
ClosedPublic

Authored by MatzeB on Apr 8 2016, 10:52 AM.

Details

Summary

It is very likely that the swiftself parameter is alive throughout most
functions function so putting it into a callee save register should
avoid spills for the callers with only a minimum amount of extra spills
in the callees.

Currently the generated code is correct but unnecessarily spills and
reloads arguments passed in callee save registers, I will address this
in upcoming patches.

This also adds a missing check that for tail calls the preserved value
of the caller must be the same as the callees parameter.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 53040.Apr 8 2016, 10:52 AM
MatzeB retitled this revision from to X86: Use a callee save register for the swiftself parameter..
MatzeB updated this object.
MatzeB added reviewers: qcolombet, manmanren.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
mgrang added a subscriber: mgrang.Apr 8 2016, 10:58 AM
mgrang added inline comments.
lib/Target/X86/X86CallingConv.td
297 ↗(On Diff #53040)

Type: Should be SwiftSelf instead of SwiftSwlf.

297 ↗(On Diff #53040)

Typo*

test/CodeGen/X86/swiftself.ll
2 ↗(On Diff #53040)

We can get rid of --check-prefix=CHECK

MatzeB updated this revision to Diff 53043.Apr 8 2016, 11:01 AM
MatzeB marked 2 inline comments as done.

Fix typo, remove unnecessary --check-prefix argument.

qcolombet added inline comments.Apr 8 2016, 11:18 AM
lib/Target/X86/X86FrameLowering.cpp
1889 ↗(On Diff #53040)

Perhaps add that this is conservatively correct but maybe not optimal.

test/CodeGen/X86/swiftself.ll
2 ↗(On Diff #53040)

Could you re-add the i386 tests?

MatzeB updated this revision to Diff 53050.Apr 8 2016, 11:21 AM

Fix "OPT:" check lines in test.

MatzeB added inline comments.Apr 8 2016, 11:26 AM
test/CodeGen/X86/swiftself.ll
2 ↗(On Diff #53050)

Is it worth testing a calling a convention that does not behave any different for SwiftSelf? We're not testing fastcall or GHC either...

MatzeB updated this revision to Diff 53070.Apr 8 2016, 1:02 PM

Add a comment that omtting the kill flag is conservatively correct.

MatzeB updated this revision to Diff 53352.Apr 11 2016, 7:55 PM
MatzeB updated this object.

Add check that for tail calls the preserved value of the caller is the same as the callees parameter.

MatzeB updated this revision to Diff 53358.Apr 11 2016, 8:45 PM

Move to R13 to have swiftself/swifterror numbers next to each other...

MatzeB updated this revision to Diff 53491.Apr 12 2016, 4:27 PM

Replace sneaky tabs with whitespace.

qcolombet accepted this revision.Apr 12 2016, 5:18 PM
qcolombet edited edge metadata.

Hi Matthias,

LGTM with nits.

If you find a way to make the code that you added available for all targets that would be great. I am guessing all the targets will need to do the same checks.

No need to address that in that patch.

Cheers,
-Quentin

lib/Target/X86/X86ISelLowering.cpp
3859 ↗(On Diff #53358)

Invert the condition and continue (LLVM coding style).

3861 ↗(On Diff #53358)

Maybe add an example that uses the variables defined here.
E.g.,
foo:
ArgReg = COPY Reg ; Foo needs to preserve Reg.
Reg = def ; Make sure def is a copy of ArgReg.
call tailcallCandidate

3865 ↗(On Diff #53358)

tabs :)

3869 ↗(On Diff #53358)

Is there a way to have this check somewhere in the generic part? Maybe even just available as a utility function somewhere?

I am guessing that all targets would need to do something similar if they pass argument in a callee saved register.

This revision is now accepted and ready to land.Apr 12 2016, 5:18 PM
MatzeB marked 8 inline comments as done.Apr 13 2016, 2:42 PM

Thanks for the review.

lib/Target/X86/X86ISelLowering.cpp
3869 ↗(On Diff #53491)

Yes should be possible to factor this out into TargetLower, I will do an NFC refactoring commit afterwards.

This revision was automatically updated to reflect the committed changes.