This is an archive of the discontinued LLVM Phabricator instance.

[IPRA] Set callee saved registers to none for local function when IPRA is enabled.
ClosedPublic

Authored by vivekvpandya on Jun 21 2016, 10:46 AM.

Details

Summary

IPRA try to optimize caller saved register by propagating register usage information from callee to caller so it is beneficial to have caller saved registers compare to callee saved registers when IPRA is enabled. Please find more detailed explanation here https://groups.google.com/d/msg/llvm-dev/XRzGhJ9wtZg/tjAJqb0eEgAJ.
This change makes local function do not have any callee preserved register when IPRA is enabled. A simple test case is also added to verify this change.

Diff Detail

Event Timeline

vivekvpandya retitled this revision from to [IPRA] Set callee saved registers to none for local function when IPRA is enabled..
vivekvpandya updated this object.
vivekvpandya added a subscriber: llvm-commits.
MatzeB added inline comments.Jun 21 2016, 11:34 AM
lib/CodeGen/TargetFrameLoweringImpl.cpp
28–29

This should go into a header.

63–68

I would first test for if (UseIPRA) and only then do the MF.getFunction(), hasLocalLnkage() && !hasAddressTaken().

test/CodeGen/X86/ipra-local-linkage.ll
5–6

You can probably simplify this further by leaving out the datalayout and using target triple = "x86_64--"

8

The metadata #0 referenced here doesn't exist, I am surprised this even parses...

vivekvpandya added inline comments.Jun 21 2016, 11:41 AM
test/CodeGen/X86/ipra-local-linkage.ll
8

oh sorry that is there due to copy paste from similar earlier test case.

vivekvpandya marked 5 inline comments as done.Jun 21 2016, 12:36 PM

Changes as per reviews.

MatzeB added inline comments.Jun 21 2016, 2:02 PM
include/llvm/Target/TargetFrameLowering.h
21–22 ↗(On Diff #61426)

This thing is defined TargetPassConfig.cpp so I would rather expect the declaration in TargetPassConfig.h

test/CodeGen/X86/ipra-local-linkage.ll
9

Do you actually need the nounwind attribute here, I would assume you can just remove it.

vivekvpandya marked 2 inline comments as done.Jun 21 2016, 9:04 PM

Few more changes as per reviews.

vivekvpandya planned changes to this revision.Jun 22 2016, 7:18 AM

After this patch test-suite has some runtime failures. Need to work on this.

The bug related to tail calls and recursive function is solved. The test case is updated as per discussion with @qcolombet. A statistic added to count number of function optimized for not having callee saved registers.

mehdi_amini added inline comments.Jul 5 2016, 12:22 PM
include/llvm/CodeGen/TargetPassConfig.h
23

I'm not sure it is fine to have a cl::opt in such a global header? If we need the option available broadly we may have to make it a member of TargetPassConfig initialized from a private cl::opt.
(I haven't look in detail where it is needed though).

include/llvm/Target/TargetFrameLowering.h
350 ↗(On Diff #62702)

Why not looking at the users of F?

lib/CodeGen/RegUsageInfoCollector.cpp
123 ↗(On Diff #62702)

You created F but didn't update uses of MF.getFunction()

129 ↗(On Diff #62702)

Add the function name in the debug output.

lib/CodeGen/TargetFrameLoweringImpl.cpp
86
if (UseIPRA && isSafeForNoCSROpt(MF.getFunction()))
  return;
test/CodeGen/X86/ipra-local-linkage.ll
9

Hasn't been fixed apparently.

20

Take %X by argument?

23

saved before and reloaded after?

vivekvpandya marked 4 inline comments as done.Jul 6 2016, 1:20 AM
vivekvpandya added inline comments.
test/CodeGen/X86/ipra-local-linkage.ll
20

Do you mean as reference?

23

yes value should be saved if register containing original value is also getting clobbered with function call to foo() but here R15 is having immediate value so it is just getting reloaded.

vivekvpandya added inline comments.Jul 6 2016, 1:22 AM
test/CodeGen/X86/ipra-local-linkage.ll
9

Yes, it has fixed, Sorry that come again as I modified an older version of it while doign git reset --soft ...

vivekvpandya added inline comments.Jul 6 2016, 1:31 AM
include/llvm/CodeGen/TargetPassConfig.h
23

UseIPRA is used in TargetPassConfig and TargetFrameLoweringImpl

vivekvpandya added inline comments.Jul 6 2016, 1:36 AM
test/CodeGen/X86/ipra-local-linkage.ll
23

But yes I will edit comment accordingly to explain it clearly.

vivekvpandya added inline comments.Jul 6 2016, 1:38 AM
include/llvm/CodeGen/TargetPassConfig.h
23

@MatzeB could you please provide some inputs?

mehdi_amini added inline comments.Jul 6 2016, 11:09 AM
include/llvm/Target/TargetFrameLowering.h
350 ↗(On Diff #62702)

Why has this been marked as done?

lib/CodeGen/RegUsageInfoCollector.cpp
123 ↗(On Diff #62702)

Why has this been marked as done?

129 ↗(On Diff #62702)

Why has this been marked as done?

lib/CodeGen/TargetFrameLoweringImpl.cpp
86

Why has this been marked as done?

test/CodeGen/X86/ipra-local-linkage.ll
20

I mean as a function parameter, unless I'm missing why an addition is needed here.

vivekvpandya added inline comments.Jul 6 2016, 11:14 AM
include/llvm/Target/TargetFrameLowering.h
350 ↗(On Diff #62702)

Changes have been done on local machine, just waiting for your decision on if UseIPRA to be in TargetPassConfig.h or not?

test/CodeGen/X86/ipra-local-linkage.ll
20

Yes addition is not needed I can change load constant value to variable X.

mehdi_amini added inline comments.Jul 6 2016, 11:16 AM
include/llvm/Target/TargetFrameLowering.h
350 ↗(On Diff #62702)

Please mark as done when the revision is updated, otherwise it makes it harder to track.

test/CodeGen/X86/ipra-local-linkage.ll
20

Why do you need a load? Why don't you take X as a function parameter (you haven't address this question...)

vivekvpandya added inline comments.Jul 6 2016, 11:19 AM
test/CodeGen/X86/ipra-local-linkage.ll
20

Do you mean as a function parameter of bar() ?

vivekvpandya added inline comments.Jul 6 2016, 11:30 AM
test/CodeGen/X86/ipra-local-linkage.ll
20

Yes, that also worked. However I have just never thought to use that. But I guess that simplified the test further. In previous version constant value and add instruction were just irrelevant.

changes as per reviews.

If this seems good then can someone commit this?

mehdi_amini accepted this revision.Jul 12 2016, 1:15 PM
mehdi_amini edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 12 2016, 1:15 PM

Please rebase.

I did git pull and this patch applied successfully with out any conflicts.

This revision was automatically updated to reflect the committed changes.