[AMDGPU] Spill CSR VGPR which is reserved for SGPR spills

[kerbowa]

Summary

A CSR VGPR being reserved for SGPR spills could be clobbered if there were no
free lower VGPR's available. Create a stack object so that it will be spilled
in the prologue.

Update the test to include a stack object so that a VGPR will be pre-reserved.

[kerbowa]

Workaround MIR emitter issue.

[arsenm]

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

361

I think checking for isEntryFunction is redundant with checking for CSRegs. Why does this need to worry about calls? This will miss the tail call case.

Can you also comment this?

llvm/test/CodeGen/AMDGPU/reserve-vgpr-for-sgpr-spill.ll

19

Can you also add a test that only has a tail call?

[kerbowa]

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

361

The logic is the same for other CSRegs being reserved for SGPR spills.

I'm not sure in what situation this might miss the tail call case? If there is a tail call and we don't clobber any SGPRs there will be no reserved VGPR and no unnecessary spill.

[kerbowa]

Create the stack object in lowerShiftReservedVGPR. Also handle this reserved VGPR not being live-in to all BB. Add more tests. Cleanup some of the existing logic.

[saiislam]

LGTM. Please wait for @arsenm 's review.

[arsenm]

LGTM with nit. I really need to get back to the register allocation split patches

llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp

241

Extra whitespace change

llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp

 // Find lowest available VGPR and use it as VGPR reserved for SGPR spills.
 static bool lowerShiftReservedVGPR(MachineFunction &MF,
                                    const GCNSubtarget &ST) {
   MachineRegisterInfo &MRI = MF.getRegInfo();
   MachineFrameInfo &FrameInfo = MF.getFrameInfo();
   SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
   Register LowestAvailableVGPR, ReservedVGPR;
   ArrayRef<MCPhysReg> AllVGPR32s = ST.getRegisterInfo()->getAllVGPR32(MF);
   for (MCPhysReg Reg : AllVGPR32s) {
     if (MRI.isAllocatable(Reg) && !MRI.isPhysRegUsed(Reg)) {
       LowestAvailableVGPR = Reg;
       break;
     }
   }
 
   if (!LowestAvailableVGPR)
     return false;
 
   ReservedVGPR = FuncInfo->VGPRReservedForSGPRSpill;
   const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();
   int i = 0;
 
   for (MachineBasicBlock &MBB : MF) {
     for (auto Reg : FuncInfo->getSGPRSpillVGPRs()) {
       if (Reg.VGPR == ReservedVGPR) {
         MBB.removeLiveIn(ReservedVGPR);
         MBB.addLiveIn(LowestAvailableVGPR);
+
+        if (Reg.FI)
+          MF.getFrameInfo().RemoveStackObject(*Reg.FI);
+
         Optional<int> FI;
         if (FuncInfo->isCalleeSavedReg(CSRegs, LowestAvailableVGPR))
           FI = FrameInfo.CreateSpillStackObject(4, Align(4));
 
         FuncInfo->setSGPRSpillVGPRs(LowestAvailableVGPR, FI, i);
       }
       ++i;
     }

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

bool SIMachineFunctionInfo::allocateSGPRSpillToVGPR(MachineFunction &MF,
   return true;
 }
 
 /// Reserve a VGPR for spilling of SGPRs
 bool SIMachineFunctionInfo::reserveVGPRforSGPRSpills(MachineFunction &MF) {
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
   const SIRegisterInfo *TRI = ST.getRegisterInfo();
   SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+  MachineFrameInfo &FrameInfo = MF.getFrameInfo();
+
+  const MCPhysReg *CSRegs = MRI.getCalleeSavedRegs();
 
   Register LaneVGPR = TRI->findUnusedRegister(
       MF.getRegInfo(), &AMDGPU::VGPR_32RegClass, MF, true);
-  SpillVGPRs.push_back(SGPRSpillVGPRCSR(LaneVGPR, None));
+
+  Optional<int> CSRSpillFI;
+  if ((FrameInfo.hasCalls() || !isEntryFunction()) && CSRegs &&
+      isCalleeSavedReg(CSRegs, LaneVGPR)) {
+    CSRSpillFI = FrameInfo.CreateSpillStackObject(4, Align(4));
+  }
+
+  SpillVGPRs.push_back(SGPRSpillVGPRCSR(LaneVGPR, CSRSpillFI));
   FuncInfo->VGPRReservedForSGPRSpill = LaneVGPR;
   return true;
 }
 
 /// Reserve AGPRs or VGPRs to support spilling for FrameIndex \p FI.
 /// Either AGPR is spilled to VGPR to vice versa.
 /// Returns true if a \p FI can be eliminated completely.
 bool SIMachineFunctionInfo::allocateVGPRSpillToAGPR(MachineFunction &MF,
bool SIMachineFunctionInfo::initializeBaseYamlFields(
   return false;
 }
 
 // Remove VGPR which was reserved for SGPR spills if there are no spilled SGPRs
 bool SIMachineFunctionInfo::removeVGPRForSGPRSpill(Register ReservedVGPR,
                                                    MachineFunction &MF) {
   for (auto *i = SpillVGPRs.begin(); i < SpillVGPRs.end(); i++) {
     if (i->VGPR == ReservedVGPR) {
+      if (i->FI)
+        MF.getFrameInfo().RemoveStackObject(*i->FI);
+
       SpillVGPRs.erase(i);
 
       for (MachineBasicBlock &MBB : MF) {
         MBB.removeLiveIn(ReservedVGPR);
         MBB.sortUniqueLiveIns();
       }
       this->VGPRReservedForSGPRSpill = AMDGPU::NoRegister;
       return true;
     }
   }
   return false;
 }

llvm/test/CodeGen/AMDGPU/fix-frame-ptr-reg-copy-livein.ll

-; RUN: llc -march=amdgcn -mcpu=gfx900 -stop-after=prologepilog -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -stop-after=prologepilog -amdgpu-reserve-vgpr-for-sgpr-spill=false -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
 ; It is a small loop test that iterates over the array member of the structure argument  passed byval to the function.
 ; The loop code will keep the prologue and epilogue blocks apart.
 ; The test is primarily to check the temp register used to preserve the earlier FP value
 ; is live-in at every BB in the function.
 
 %struct.Data = type { [20 x i32] }
 

llvm/test/CodeGen/AMDGPU/reserve-vgpr-for-sgpr-spill.ll

 ; RUN: llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx803 -O0 -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=GCN %s
 
 define void @child_function() #0 {
   call void asm sideeffect "", "~{vcc}" () #0
   ret void
 }
 
 ; GCN-LABEL: {{^}}parent_func:
-; CHECK:  v_writelane_b32 v255, s33, 2
-; CHECK:  v_writelane_b32 v255, s30, 0
-; CHECK:  v_writelane_b32 v255, s31, 1
+; CHECK:  buffer_store_dword v255, off, s[0:3], s32
+; CHECK:  v_writelane_b32 v255, s33, 2
+; CHECK:  v_writelane_b32 v255, s30, 0
+; CHECK:  v_writelane_b32 v255, s31, 1
 ; CHECK:  s_swappc_b64 s[30:31], s[4:5]
-; CHECK:  v_readlane_b32 s4, v255, 0
+; CHECK:  v_readlane_b32 s4, v255, 0
 ; CHECK:  v_readlane_b32 s5, v255, 1
-; CHECK:  v_readlane_b32 s33, v255, 2
+; CHECK:  v_readlane_b32 s33, v255, 2
 ; GCN: ; NumVgprs: 256
 
 define void @parent_func() #0 {
+  %alloca = alloca i32, align 4, addrspace(5)
+  store volatile i32 0, i32 addrspace(5)* %alloca
+
   call void asm sideeffect "",
   "~{v0},~{v1},~{v2},~{v3},~{v4},~{v5},~{v6},~{v7},~{v8},~{v9}
   ,~{v10},~{v11},~{v12},~{v13},~{v14},~{v15},~{v16},~{v17},~{v18},~{v19}
   ,~{v20},~{v21},~{v22},~{v23},~{v24},~{v25},~{v26},~{v27},~{v28},~{v29}
   ,~{v30},~{v31},~{v32},~{v33},~{v34},~{v35},~{v36},~{v37},~{v38},~{v39}
   ,~{v40},~{v41},~{v42},~{v43},~{v44},~{v45},~{v46},~{v47},~{v48},~{v49}
   ,~{v50},~{v51},~{v52},~{v53},~{v54},~{v55},~{v56},~{v57},~{v58},~{v59}
   ,~{v60},~{v61},~{v62},~{v63},~{v64},~{v65},~{v66},~{v67},~{v68},~{v69}