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

[vivekvpandya]

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.

[MatzeB]

lib/CodeGen/TargetFrameLoweringImpl.cpp

27–28

This should go into a header.

64–69

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

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

4–5

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

7

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

[vivekvpandya]

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

7

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

[vivekvpandya]

Changes as per reviews.

[MatzeB]

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]

Few more changes as per reviews.

[vivekvpandya]

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

[vivekvpandya]

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]

include/llvm/CodeGen/TargetPassConfig.h

23 ↗

(On Diff #62702)

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]

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]

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]

include/llvm/CodeGen/TargetPassConfig.h

23 ↗

(On Diff #62702)

UseIPRA is used in TargetPassConfig and TargetFrameLoweringImpl

[vivekvpandya]

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

23

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

[vivekvpandya]

include/llvm/CodeGen/TargetPassConfig.h

23 ↗

(On Diff #62702)

@MatzeB could you please provide some inputs?

[mehdi_amini]

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]

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]

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]

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

20

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

[vivekvpandya]

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.

[vivekvpandya]

changes as per reviews.

[vivekvpandya]

If this seems good then can someone commit this?

[mehdi_amini]

LGTM.

[mehdi_amini]

Please rebase.

[vivekvpandya]

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

lib/CodeGen/TargetFrameLoweringImpl.cpp

 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Function.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 #include <cstdlib>
 using namespace llvm;
 
+extern cl::opt<bool> UseIPRA;
+
 TargetFrameLowering::~TargetFrameLowering() {
 }
 
 /// The default implementation just looks at attribute "no-frame-pointer-elim".
 bool TargetFrameLowering::noFramePointerElim(const MachineFunction &MF) const {
   auto Attr = MF.getFunction()->getFnAttribute("no-frame-pointer-elim");
   return Attr.getValueAsString() == "true";
 }

 bool TargetFrameLowering::needsFrameIndexResolution(
     const MachineFunction &MF) const {
   return MF.getFrameInfo()->hasStackObjects();
 }
 
 void TargetFrameLowering::determineCalleeSaves(MachineFunction &MF,
                                                BitVector &SavedRegs,
                                                RegScavenger *RS) const {
+  Function *F = const_cast<Function *>(MF.getFunction());
+
+  // When interprocedural register allocation is enabled caller saved registers
+  // are preferred over callee saved registers.
+  if (UseIPRA && F->hasLocalLinkage() && !F->hasAddressTaken())
+    return;
+
   // Get the callee saved register list...
   const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
   const MCPhysReg *CSRegs = TRI.getCalleeSavedRegs(&MF);
 
   // Resize before the early returns. Some backends expect that
   // SavedRegs.size() == TRI.getNumRegs() after this call even if there are no
   // saved registers.
   SavedRegs.resize(TRI.getNumRegs());
 
   // Early exit if there are no callee saved registers.
   if (!CSRegs || CSRegs[0] == 0)
     return;
 
   // In Naked functions we aren't going to save any registers.
   if (MF.getFunction()->hasFnAttribute(Attribute::Naked))
     return;
 
   // Functions which call __builtin_unwind_init get all their registers saved.
   bool CallsUnwindInit = MF.getMMI().callsUnwindInit();
   const MachineRegisterInfo &MRI = MF.getRegInfo();
   for (unsigned i = 0; CSRegs[i]; ++i) {
     unsigned Reg = CSRegs[i];
     if (CallsUnwindInit || MRI.isPhysRegModified(Reg))
       SavedRegs.set(Reg);

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

+; RUN: llc < %s | FileCheck %s -check-prefix=NOIPRA
+; RUN: llc -enable-ipra < %s | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12.0"
+
+define void @bar() #0 {
+  call void @foo()
+  ret void
+}
+
+define internal void @foo() #0 {
+; When IPRA is not enabled R15 will be saved by foo as it is callee saved reg.
+; NOIPRA-LABEL: foo:
+; NOIPRA: pushq %r15
+; When IPRA is enabled none register should be saved as foo() is local function
+; so we optimize it to save no registers.
+; CHECK: foo:
+; CHECK-NOT: pushq %r15
+  call void asm sideeffect "", "~{eax},~{ecx},~{edi},~{r15}"() #0
+  ret void
+}