AMDGPU: Don't enable all lanes with non-CSR VGPR spills

[arsenm]

Summary

If the only VGPRs used for SGPR spilling were not CSRs, this was
enabling all laness and immediately restoring exec. This is the usual
situation in leaf functions.

[rampitec]

LGTM, but I still believe it shall be reserved register instead.

[arsenm]

In D62519#1519357, @rampitec wrote:

LGTM, but I still believe it shall be reserved register instead.

I'm not sure what you mean. A reserved register wouldn't help

[rampitec]

In D62519#1519360, @arsenm wrote:

In D62519#1519357, @rampitec wrote:

LGTM, but I still believe it shall be reserved register instead.

I'm not sure what you mean. A reserved register wouldn't help

It would help if we can reserve it in both caller and callee, right?

[arsenm]

In D62519#1519365, @rampitec wrote:

In D62519#1519360, @arsenm wrote:

In D62519#1519357, @rampitec wrote:

LGTM, but I still believe it shall be reserved register instead.

I'm not sure what you mean. A reserved register wouldn't help

It would help if we can reserve it in both caller and callee, right?

Not sure what register you mean. Reserving registers generally causes more problems that it solves. Reserving VGPRs for SGPR spills would require reserving 2 to cover all possible SGPR spills

[rampitec]

In D62519#1519387, @arsenm wrote:

In D62519#1519365, @rampitec wrote:

In D62519#1519360, @arsenm wrote:

In D62519#1519357, @rampitec wrote:

LGTM, but I still believe it shall be reserved register instead.

I'm not sure what you mean. A reserved register wouldn't help

It would help if we can reserve it in both caller and callee, right?

Not sure what register you mean. Reserving registers generally causes more problems that it solves. Reserving VGPRs for SGPR spills would require reserving 2 to cover all possible SGPR spills

I mean we can do it conditionally if we have a call stack. Normally you would only need one VGPR, but again that requires whole program analysis.

[arsenm]

In D62519#1519393, @rampitec wrote:

In D62519#1519387, @arsenm wrote:

In D62519#1519365, @rampitec wrote:

In D62519#1519360, @arsenm wrote:

In D62519#1519357, @rampitec wrote:

LGTM, but I still believe it shall be reserved register instead.

I'm not sure what you mean. A reserved register wouldn't help

It would help if we can reserve it in both caller and callee, right?

Not sure what register you mean. Reserving registers generally causes more problems that it solves. Reserving VGPRs for SGPR spills would require reserving 2 to cover all possible SGPR spills

I mean we can do it conditionally if we have a call stack. Normally you would only need one VGPR, but again that requires whole program analysis.

I think for a recursive call, you could need more than 2 VGPRs, so eventually you would still need to spill one to memory

[arsenm]

r361848

[rampitec]

JBTW, strictly speaking you do not need a full wave and 256 bytes of scratch loads and stores per VGPR. Only first min(wavefrontsize, # of SGPRs spilled) lanes are needed.

lib/Target/AMDGPU/SIFrameLowering.cpp

void SIFrameLowering::emitPrologue(MachineFunction &MF,
 
   if (RoundedSize != 0 && hasSP(MF)) {
     BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_ADD_U32), StackPtrReg)
       .addReg(StackPtrReg)
       .addImm(RoundedSize * ST.getWavefrontSize())
       .setMIFlag(MachineInstr::FrameSetup);
   }
 
-  if (!FuncInfo->getSGPRSpillVGPRs().empty()) {
+  // To avoid clobbering VGPRs in lanes that weren't active on function entry,
+  // turn on all lanes before doing the spill to memory.
+  unsigned ScratchExecCopy = AMDGPU::NoRegister;
+
+  for (const SIMachineFunctionInfo::SGPRSpillVGPRCSR &Reg
+         : FuncInfo->getSGPRSpillVGPRs()) {
+    if (!Reg.FI.hasValue())
+      continue;
+
+    if (ScratchExecCopy == AMDGPU::NoRegister) {
       if (LiveRegs.empty()) {
         LiveRegs.init(TRI);
         LiveRegs.addLiveIns(MBB);
       }
 
-    // To avoid clobbering VGPRs in lanes that weren't active on function entry,
-    // turn on all lanes before doing the spill to memory.
-    unsigned ScratchExecCopy
+      ScratchExecCopy
         = findScratchNonCalleeSaveRegister(MF, LiveRegs,
                                            AMDGPU::SReg_64_XEXECRegClass);
 
-    BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_OR_SAVEEXEC_B64), ScratchExecCopy)
+      BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_OR_SAVEEXEC_B64),
+              ScratchExecCopy)
         .addImm(-1);
+    }
 
-    for (const SIMachineFunctionInfo::SGPRSpillVGPRCSR &Reg
-           : FuncInfo->getSGPRSpillVGPRs()) {
-      if (!Reg.FI.hasValue())
-        continue;
-      TII->storeRegToStackSlot(MBB, MBBI, Reg.VGPR, true,
-                               Reg.FI.getValue(), &AMDGPU::VGPR_32RegClass,
-                               &TII->getRegisterInfo());
-    }
 
+  if (ScratchExecCopy != AMDGPU::NoRegister) {
     // FIXME: Split block and make terminator.
     BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_MOV_B64), AMDGPU::EXEC)
       .addReg(ScratchExecCopy);
   }
 }
 
 void SIFrameLowering::emitEpilogue(MachineFunction &MF,
                                    MachineBasicBlock &MBB) const {
   const SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
   if (FuncInfo->isEntryFunction())
     return;
 
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
   const SIInstrInfo *TII = ST.getInstrInfo();
   MachineBasicBlock::iterator MBBI = MBB.getFirstTerminator();
   DebugLoc DL;
 
-  if (!FuncInfo->getSGPRSpillVGPRs().empty()) {
+  unsigned ScratchExecCopy = AMDGPU::NoRegister;
+  for (const SIMachineFunctionInfo::SGPRSpillVGPRCSR &Reg
+         : FuncInfo->getSGPRSpillVGPRs()) {
+    if (!Reg.FI.hasValue())
+      continue;
+
+    if (ScratchExecCopy == AMDGPU::NoRegister) {
       // See emitPrologue
       LivePhysRegs LiveRegs(*ST.getRegisterInfo());
       LiveRegs.addLiveIns(MBB);
 
-    unsigned ScratchExecCopy
+      ScratchExecCopy
         = findScratchNonCalleeSaveRegister(MF, LiveRegs,
                                            AMDGPU::SReg_64_XEXECRegClass);
 
       BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_OR_SAVEEXEC_B64), ScratchExecCopy)
         .addImm(-1);
+    }
 
-    for (const SIMachineFunctionInfo::SGPRSpillVGPRCSR &Reg
-           : FuncInfo->getSGPRSpillVGPRs()) {
-      if (!Reg.FI.hasValue())
-        continue;
-      TII->loadRegFromStackSlot(MBB, MBBI, Reg.VGPR,
-                                Reg.FI.getValue(), &AMDGPU::VGPR_32RegClass,
-                                &TII->getRegisterInfo());
-    }
 
+  if (ScratchExecCopy != AMDGPU::NoRegister) {
     // FIXME: Split block and make terminator.
     BuildMI(MBB, MBBI, DL, TII->get(AMDGPU::S_MOV_B64), AMDGPU::EXEC)
       .addReg(ScratchExecCopy);
   }
 
   unsigned StackPtrReg = FuncInfo->getStackPtrOffsetReg();
   if (StackPtrReg == AMDGPU::NoRegister)
     return;

test/CodeGen/AMDGPU/callee-frame-setup.ll

define void @callee_func_sgpr_spill_no_calls(i32 %in) #0 {
   call void asm sideeffect "; use $0", "s"(<16 x i32> %wide.sgpr1) #0
   call void asm sideeffect "; use $0", "s"(<16 x i32> %wide.sgpr2) #0
   call void asm sideeffect "; use $0", "s"(<8 x i32> %wide.sgpr3) #0
   call void asm sideeffect "; use $0", "s"(<2 x i32> %wide.sgpr4) #0
   call void asm sideeffect "; use $0", "s"(<16 x i32> %wide.sgpr5) #0
   ret void
 }
 
+; Has no spilled CSR VGPRs used for SGPR spilling, so no need to
+; enable all lanes and restore.
+
+; GCN-LABEL: {{^}}spill_only_csr_sgpr:
+; GCN: s_waitcnt
+; GCN-NEXT: v_writelane_b32 v0, s42, 0
+; GCN-NEXT: ;;#ASMSTART
+; GCN-NEXT: ; clobber s42
+; GCN-NEXT: ;;#ASMEND
+; GCN-NEXT: v_readlane_b32 s42, v0, 0
+; GCN-NEXT: s_setpc_b64
+define void @spill_only_csr_sgpr() {
+  call void asm sideeffect "; clobber s42", "~{s42}"()
+  ret void
+}
+
 attributes #0 = { nounwind }
 attributes #1 = { nounwind "no-frame-pointer-elim"="true" }