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

124

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 ↗

(On Diff #43765)

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 ↗

(On Diff #43765)

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/SIFrameLowering.cpp

void SIFrameLowering::emitPrologue(MachineFunction &MF,
 
   // 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)) {
+    unsigned Drop = 2;
+
+    if (ST.hasSGPRInitBug())
+      Drop = 8;
+
+    for (MCPhysReg Reg : getAllSGPR128().drop_back(Drop).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;