AMDGPU: Avoid saving/restoring reserved m0 register

[MatzeB]

Summary

This is in preparation to https://reviews.llvm.org/D23097 which uncovered some problems in the way AMDGPU uses the register scavenger in SIRegisterInfo::spillSGPR()/SIRegisterInfo::restoreSGPR().

• The current code does not use the register scavenger correctly:
  • RS->isRegUsed() always reports true for m0 because it is a reserved register.
  • Even if m0 was not reserved RS->isRegUsed() would give you information about the position where we are scavenging a register. This is not actually the position where the spill/reload will be inserted.
  • Spilling m0 creates a new vreg. This is sketchy as that happens at a time when the register scavenger is already spilling/reloading to free registers for vreg assignment. If there is no free physreg at that place we end up in an endless loop (because m0 is always reported as reserved, see above). The existing tests are lucky to not hit the case with D23097 applied one is hit.

This changes the code to assume that m0 is available since it is
reserved so we shouldn't have any longer liveranges crossing potential
spill/reload places.

[tstellar]

This changes the code to assume that m0 is available since it is
reserved so we shouldn't have any longer liveranges crossing potential
spill/reload places.

m0 could potentially be live for the whole program, so this assumption is not correct.

[MatzeB]

In D34189#779880, @tstellar wrote:

This changes the code to assume that m0 is available since it is
reserved so we shouldn't have any longer liveranges crossing potential
spill/reload places.

m0 could potentially be live for the whole program, so this assumption is not correct.

So you actually do you use the register before prolog/epilogue insertion? (Note that the register allocator will not use it at the moment as it is reserved, so it would need other AMDGPU code to explicitely use it).

[tstellar]

In D34189#779882, @MatzeB wrote:

So you actually do you use the register before prolog/epilogue insertion? (Note that the register allocator will not use it at the moment as it is reserved, so it would need other AMDGPU code to explicitely use it).

Yes, the SelectionDAG emits instructions that have implicit defs of the register, see SITargetLowering::copyToM0(), and several instructions have it defined as an implicit use. Take a look at the ds_read2.ll test case for an example.

[MatzeB]

For the record: I just checked why that testcase did not fail before my patch. It's simply hitting a bug in the register scavenger:

$ llc -mtriple=amdgcn--amdhsa -mcpu=fiji -amdgpu-spill-sgpr-to-smem=1 -print-machineinstrs test/CodeGen/AMDGPU/attr-amdgpu-num-sgpr.ll
...
# After Shrink Wrapping analysis:
# Machine code for function max_12_sgprs_14_input_sgprs: NoPHIs, TracksLiveness, NoVRegs
...
BB#1:
  ...%SGPR4_SGPR5<def> = SI_SPILL_S64_RESTORE <fi#3>, %EXEC<imp-use>, %SGPR8_SGPR9_SGPR10_SGPR11<imp-use>, %SGPR7<imp-use>, %M0<imp-def>; mem:LD8[FixedStack3](align=4)

after frame elimination/scavenging:

...
# After Prologue/Epilogue Insertion & Frame Finalization:
# Machine code for function max_12_sgprs_14_input_sgprs: NoPHIs, TracksLiveness, NoVRegs
...
#B1:
  ...
    %M0<def> = S_ADD_U32 %SGPR7, 1024, %SCC<imp-def>
    %SGPR4_SGPR5<def> = S_BUFFER_LOAD_DWORDX2_SGPR %SGPR8_SGPR9_SGPR10_SGPR11, %M0<kill>, 0; mem:LD8[FixedStack3](align=4)
    %M0<def> = COPY %SGPR4<kill>

=> This restores the wrong value into M0. (There is not free register at that point in the program so your recovery strategy of saving M0 in a register is impossible to solve at that point, but you happen to hit a bug in the old scavenging code that reports SGPR4 :-/ )

[MatzeB]

Would it be okay to XFAIL that part of attr-amdgpu-num-sgpr.ll? It is currently blocking me from moving forward with D23097

[MatzeB]

In D34189#780785, @MatzeB wrote:

but you happen to hit a bug in the old scavenging code that reports SGPR4 :-/

Turns out this is not directly a bug in the register scavenger, but comes from the fact that AMDGPU creates new vregs while the scavenger is running. When the vreg to save m0 is created in the example, we have already moved past the definition of SGPR4_SGPR5 and it will miss that the register is occupied further below; It doesn't really work in this "reentrant" way.

[tstellar]

In D34189#780788, @MatzeB wrote:

Would it be okay to XFAIL that part of attr-amdgpu-num-sgpr.ll? It is currently blocking me from moving forward with D23097

I think that's fine since it's currently passing by accident.

[MatzeB]

Abandoning.

I commented out the part of the test that failed when pushing D21885 / r305516

lib/Target/AMDGPU/SIRegisterInfo.cpp

bool SIRegisterInfo::spillSGPR(MachineBasicBlock::iterator MI,
   MachineFrameInfo &FrameInfo = MF->getFrameInfo();
 
   bool SpillToSMEM = spillSGPRToSMEM();
   if (SpillToSMEM && OnlyToVGPR)
     return false;
 
   assert(SuperReg != AMDGPU::M0 && "m0 should never spill");
 
+  assert(MRI.isReserved(AMDGPU::M0));
   unsigned OffsetReg = AMDGPU::M0;
-  unsigned M0CopyReg = AMDGPU::NoRegister;
-
-  if (SpillToSMEM) {
-    if (RS->isRegUsed(AMDGPU::M0)) {
-      M0CopyReg = MRI.createVirtualRegister(&AMDGPU::SReg_32_XM0RegClass);
-      BuildMI(*MBB, MI, DL, TII->get(AMDGPU::COPY), M0CopyReg)
-        .addReg(AMDGPU::M0);
-    }
-  }
 
   unsigned ScalarStoreOp;
   unsigned EltSize = 4;
   const TargetRegisterClass *RC = getPhysRegClass(SuperReg);
   if (SpillToSMEM && isSGPRClass(RC)) {
     // XXX - if private_element_size is larger than 4 it might be useful to be
     // able to spill wider vmem spills.
     std::tie(EltSize, ScalarStoreOp) =
if (SpillToVGPR) {
         .addFrameIndex(Index)              // vaddr
         .addReg(MFI->getScratchRSrcReg())  // srrsrc
         .addReg(MFI->getFrameOffsetReg())  // soffset
         .addImm(i * 4)                     // offset
         .addMemOperand(MMO);
     }
   }
 
-  if (M0CopyReg != AMDGPU::NoRegister) {
-    BuildMI(*MBB, MI, DL, TII->get(AMDGPU::COPY), AMDGPU::M0)
-      .addReg(M0CopyReg, RegState::Kill);
-  }
-
   MI->eraseFromParent();
   MFI->addToSpilledSGPRs(NumSubRegs);
   return true;
 }
 
 bool SIRegisterInfo::restoreSGPR(MachineBasicBlock::iterator MI,
                                  int Index,
                                  RegScavenger *RS,
bool SIRegisterInfo::restoreSGPR(MachineBasicBlock::iterator MI,
 
   unsigned SuperReg = MI->getOperand(0).getReg();
   bool SpillToSMEM = spillSGPRToSMEM();
   if (SpillToSMEM && OnlyToVGPR)
     return false;
 
   assert(SuperReg != AMDGPU::M0 && "m0 should never spill");
 
+  assert(MRI.isReserved(AMDGPU::M0));
   unsigned OffsetReg = AMDGPU::M0;
-  unsigned M0CopyReg = AMDGPU::NoRegister;
-
-  if (SpillToSMEM) {
-    if (RS->isRegUsed(AMDGPU::M0)) {
-      M0CopyReg = MRI.createVirtualRegister(&AMDGPU::SReg_32_XM0RegClass);
-      BuildMI(*MBB, MI, DL, TII->get(AMDGPU::COPY), M0CopyReg)
-        .addReg(AMDGPU::M0);
-    }
-  }
 
   unsigned EltSize = 4;
   unsigned ScalarLoadOp;
 
   const TargetRegisterClass *RC = getPhysRegClass(SuperReg);
   if (SpillToSMEM && isSGPRClass(RC)) {
     // XXX - if private_element_size is larger than 4 it might be useful to be
     // able to spill wider vmem spills.
if (SpillToVGPR) {
         BuildMI(*MBB, MI, DL, TII->get(AMDGPU::V_READFIRSTLANE_B32), SubReg)
         .addReg(TmpReg, RegState::Kill);
 
       if (NumSubRegs > 1)
         MIB.addReg(MI->getOperand(0).getReg(), RegState::ImplicitDefine);
     }
   }
 
-  if (M0CopyReg != AMDGPU::NoRegister) {
-    BuildMI(*MBB, MI, DL, TII->get(AMDGPU::COPY), AMDGPU::M0)
-      .addReg(M0CopyReg, RegState::Kill);
-  }
-
   MI->eraseFromParent();
   return true;
 }
 
 /// Special case of eliminateFrameIndex. Returns true if the SGPR was spilled to
 /// a VGPR and the stack slot can be safely eliminated when all other users are
 /// handled.
 bool SIRegisterInfo::eliminateSGPRToVGPRSpillFrameIndex(

test/CodeGen/AMDGPU/spill-m0.ll

 
 ; GCN: ; def m0, 1
 
 ; GCN: s_mov_b32 m0, s2
 ; GCN: v_interp_mov_f32
 
 ; GCN: ; clobber m0
 
-; TOSMEM: s_mov_b32 vcc_hi, m0
 ; TOSMEM: s_add_u32 m0, s3, 0x100
 ; TOSMEM-NEXT: s_buffer_store_dwordx2 s{{\[[0-9]+:[0-9]+\]}}, s{{\[[0-9]+:[0-9]+\]}}, m0 ; 8-byte Folded Spill
-; TOSMEM: s_mov_b32 m0, vcc_hi
 
 ; TOSMEM: s_mov_b64 exec,
 ; TOSMEM: s_cbranch_execz
 ; TOSMEM: s_branch
 
 ; TOSMEM: BB{{[0-9]+_[0-9]+}}:
 ; TOSMEM: s_add_u32 m0, s3, 0x100
 ; TOSMEM-NEXT: s_buffer_load_dwordx2 s{{\[[0-9]+:[0-9]+\]}}, s{{\[[0-9]+:[0-9]+\]}}, m0 ; 8-byte Folded Reload

 ; FIXME-TOSMEM-NOT: m0
 ; TOSMEM: s_add_u32 m0, s3, 0x300
 ; TOSMEM: s_buffer_store_dword s{{[0-9]+}}, s[88:91], m0 ; 4-byte Folded Spill
 ; FIXME-TOSMEM-NOT: m0
 ; TOSMEM: s_cbranch_scc1
 
 ; TOSMEM: s_mov_b32 m0, -1
 
-; TOSMEM: s_mov_b32 vcc_hi, m0
 ; TOSMEM: s_add_u32 m0, s3, 0x100
 ; TOSMEM: s_buffer_load_dwordx2 s{{\[[0-9]+:[0-9]+\]}}, s[88:91], m0 ; 8-byte Folded Reload
-; TOSMEM: s_mov_b32 m0, vcc_hi
 ; TOSMEM: s_waitcnt lgkmcnt(0)
 
 ; TOSMEM: ds_write_b64
 
 ; FIXME-TOSMEM-NOT: m0
 ; TOSMEM: s_add_u32 m0, s3, 0x300
 ; TOSMEM: s_buffer_load_dword s0, s[88:91], m0 ; 4-byte Folded Reload
 ; FIXME-TOSMEM-NOT: m0