AMDGPU/SI: fix scratch resource register allocation on Tonga & Iceland

[nhaehnle]

Summary

The correct number of available SGPRs needs to be taken into account here.

[arsenm]

Do you have a test that hits this?

This is also repeated for the ScratchWaveOffsetReg below, so that probably also needs this.

I don't really remember why I did it this way. At this point we should know exactly which registers vcc/flat_scr/xnack should be in

lib/Target/AMDGPU/SIFrameLowering.cpp

107–108

This should also account for flat_scr. I'm not sure we have a way to check if flat_scr is needed yet

[nhaehnle]

Yes, I actually ran into this with a real shader, but it's a very big one :) To hit this, all SGPRs _and_ VGPR spilling have to be used. Perhaps a long sequence of loads followed by some kind of barrier intrinsics could work?

And you're right, the code below also looks like it needs to be adapted, but somehow the bug got fixed even without it. I'll look into it.

[nhaehnle]

I have distilled a test that hits the issue. It's a pretty subtle balancing
to ensure the right number of SGPRs are used to trigger the bug without
running into the "Ran out of VGPRs for SGPR spilling" issue.

Two logically separate changes are required to make this new test pass:

1. Just skip the moving around of scratch resource registers on chips with the

SGPR init bug; after all, there's nothing to be gained by the move. This is a
simpler approach to what I tried before.

2. Separately, make getSIProgramInfo assume that XNACK_MASK is unused; this is

not really a long-term solution, but the other register calculations I've seen
(in particular getReservedRegs) make the same assumption.

• AMDGPU: add test/CodeGen/AMDGPU/spill-stress.ll
• AMDGPU/SI: Do not move scratch resource register on Tonga & Iceland
• AMDGPU/SI: Assume XNACK_MASK is not used in getSIProgramInfo

[nhaehnle]

Reverted an incorrect hunk in getSIProgramInfo

• AMDGPU/SI: Assume XNACK_MASK is not used in getSIProgramInfo

[• tstellarAMD]

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

426–428

I would prefer if we future proofed this by adding the XNACK registers and checking for them like we do VCC and Flat.

test/CodeGen/AMDGPU/spill-stress.ll

1

You may be able to write a better test using inline assembly. Take a look at the flat-scratch-reg.ll test for an example.

[nhaehnle]

Thanks for the hint regarding inline assembly, that makes the test much nicer.

I dropped the part about XNACK from this patch. With only this patch applied,
the new test should actually still fail, though with a different error message.
The test passes when D15869 is applied as well.

• AMDGPU: add test/CodeGen/AMDGPU/spill-stress.ll
• AMDGPU/SI: Do not move scratch resource register on Tonga & Iceland

[• tstellarAMD]

LGTM.

[nhaehnle]

This is r256870.

Not sure why it wasn't closed automatically, I did add the Differential Revision comment after squashing the patches I had locally. It seems SVN is having problems right now, perhaps that's why the bot hasn't picked it up.

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

void AMDGPUAsmPrinter::getSIProgramInfo(SIProgramInfo &ProgInfo,
   }
 
   if (VCCUsed || FlatUsed)
     MaxSGPR += 2;
 
   if (FlatUsed) {
     MaxSGPR += 2;
     // 2 additional for VI+.
-    if (STM.getGeneration() >= AMDGPUSubtarget::VOLCANIC_ISLANDS)
-      MaxSGPR += 2;
+    if (STM.getGeneration() >= AMDGPUSubtarget::VOLCANIC_ISLANDS) {
+      // Assume XNACK_MASK is not used
+      //MaxSGPR += 2;
+    }
   }
 
   // We found the maximum register index. They start at 0, so add one to get the
   // number of registers.
   ProgInfo.NumVGPR = MaxVGPR + 1;
   ProgInfo.NumSGPR = MaxSGPR + 1;
 
   if (STM.hasSGPRInitBug()) {

lib/Target/AMDGPU/SIFrameLowering.cpp

void SIFrameLowering::emitPrologue(MachineFunction &MF,
   MachineRegisterInfo &MRI = MF.getRegInfo();
   MRI.addLiveIn(PreloadedScratchWaveOffsetReg);
   MBB.addLiveIn(PreloadedScratchWaveOffsetReg);
 
   if (ST.isAmdHsaOS()) {
     MRI.addLiveIn(PreloadedPrivateBufferReg);
     MBB.addLiveIn(PreloadedPrivateBufferReg);
   }
 
+  if (!ST.hasSGPRInitBug()) {
     // We reserved the last registers for this. Shift it down to the end of those
     // which were actually used.
     //
     // FIXME: It might be safer to use a pseudoregister before replacement.
 
     // FIXME: We should be able to eliminate unused input registers. We only
     // cannot do this for the resources required for scratch access. For now we
     // skip over user SGPRs and may leave unused holes.
 
     // We find the resource first because it has an alignment requirement.
     if (ScratchRsrcReg == TRI->reservedPrivateSegmentBufferReg(MF)) {
       MachineRegisterInfo &MRI = MF.getRegInfo();
 
       unsigned NumPreloaded = MFI->getNumPreloadedSGPRs() / 4;
       // Skip the last 2 elements because the last one is reserved for VCC, and
       // this is the 2nd to last element already.
       for (MCPhysReg Reg : getAllSGPR128().drop_back(2).slice(NumPreloaded)) {
         // Pick the first unallocated one. Make sure we don't clobber the other
         // reserved input we needed.
         if (!MRI.isPhysRegUsed(Reg)) {
           assert(MRI.isAllocatable(Reg));
           MRI.replaceRegWith(ScratchRsrcReg, Reg);
           ScratchRsrcReg = Reg;
           MFI->setScratchRSrcReg(ScratchRsrcReg);
           break;
         }
       }
     }
 
     if (ScratchWaveOffsetReg == TRI->reservedPrivateSegmentWaveByteOffsetReg(MF)) {
       MachineRegisterInfo &MRI = MF.getRegInfo();
       // Skip the last 2 elements because the last one is reserved for VCC, and
       // this is the 2nd to last element already.
       unsigned NumPreloaded = MFI->getNumPreloadedSGPRs();
       for (MCPhysReg Reg : getAllSGPRs().drop_back(6).slice(NumPreloaded)) {
         // Pick the first unallocated SGPR. Be careful not to pick an alias of the
         // scratch descriptor, since we haven’t added its uses yet.
         if (!MRI.isPhysRegUsed(Reg)) {
           assert(MRI.isAllocatable(Reg) &&
                 !TRI->isSubRegisterEq(ScratchRsrcReg, Reg));
 
           MRI.replaceRegWith(ScratchWaveOffsetReg, Reg);
           ScratchWaveOffsetReg = Reg;
           MFI->setScratchWaveOffsetReg(ScratchWaveOffsetReg);
           break;
         }
       }
     }
+  }
 
 
   assert(!TRI->isSubRegister(ScratchRsrcReg, ScratchWaveOffsetReg));
 
   const MCInstrDesc &SMovB32 = TII->get(AMDGPU::S_MOV_B32);
   MachineBasicBlock::iterator I = MBB.begin();
   DebugLoc DL;
 

test/CodeGen/AMDGPU/spill-stress.ll

+; RUN: llc -march=amdgcn -mcpu=tonga < %s | FileCheck --check-prefix=TONGA %s
+
+; On Tonga and Iceland, limited SGPR availability means care must be taken to
+; allocate scratch registers correctly.
+; TONGA-LABEL: test
+define void @test(<60 x i32> addrspace(1)* %out, i32 addrspace(1)* %foil, <60 x i32> addrspace(1)* %in, <60 x i32> %f) {
+entry:
+  %tid = call i32 @llvm.SI.tid() nounwind readnone
+  %aptr = getelementptr <60 x i32>, <60 x i32> addrspace(1)* %in, i32 %tid
+  %a = load <60 x i32>, <60 x i32> addrspace(1)* %aptr
+  store i32 %tid, i32 addrspace(1)* %foil
+  %bidx = add i32 %tid, 60
+  %bptr = getelementptr <60 x i32>, <60 x i32> addrspace(1)* %in, i32 %bidx
+  %b = load <60 x i32>, <60 x i32> addrspace(1)* %bptr
+  %bfoil = getelementptr i32, i32 addrspace(1)* %foil, i32 1
+  store i32 %bidx, i32 addrspace(1)* %bfoil
+  %cidx = add i32 %tid, 128
+  %cptr = getelementptr <60 x i32>, <60 x i32> addrspace(1)* %in, i32 %cidx
+  %c = load <60 x i32>, <60 x i32> addrspace(1)* %cptr
+  %cfoil = getelementptr i32, i32 addrspace(1)* %bfoil, i32 1
+  store i32 %cidx, i32 addrspace(1)* %cfoil
+  %didx = add i32 %tid, 196
+  %dptr = getelementptr <60 x i32>, <60 x i32> addrspace(1)* %in, i32 %didx
+  %d = load <60 x i32>, <60 x i32> addrspace(1)* %dptr
+  %dfoil = getelementptr i32, i32 addrspace(1)* %cfoil, i32 1
+  store i32 %cidx, i32 addrspace(1)* %dfoil
+  %eidx = add i32 %tid, 256
+  %eptr = getelementptr <60 x i32>, <60 x i32> addrspace(1)* %in, i32 %eidx
+  %e = load <60 x i32>, <60 x i32> addrspace(1)* %eptr
+  %am = mul <60 x i32> %a, %f
+  %s1 = add <60 x i32> %b, %am
+  %s2 = add <60 x i32> %s1, %c
+  %s3 = add <60 x i32> %s2, %d
+  %s = add <60 x i32> %e, %s3
+  %outptr = getelementptr <60 x i32>, <60 x i32> addrspace(1)* %out, i32 %tid
+  store <60 x i32> %s, <60 x i32> addrspace(1)* %outptr
+  ret void
+}
+
+declare i32 @llvm.SI.tid() nounwind readnone