R600/SI: Fix bug in VGPR spilling

[• tstellarAMD]

Summary

AMDGPU::SI_SPILL_V96_RESTORE was missing from a switch statement, which caused the srsrc and soffset register to not be set correctly.

This commit replaces the switch statement with a SITargetInfo query to make sure all spill instructions are covered.

[arsenm]

LGTM, although it could use a test

[• tstellarAMD]

The only test case I can get for this is too big and very fragile. The problem is that this bug fixes a spill of a VGPR96 register. The only way to create a def of a VGPR96 register is to have an image sample instruction whose only uses in the same block are either extract_element or a machine instruction. Compiling at -O2, the backend will select the VGPR96 image sample and then later on sink the extract elements, which gives us a VGPR96 live out that we could normally force spill by compiling with -O0. However, at -O0 the extract elements do not get sunk, so we end up spilling 3 32-bit values instead.

llvm/trunk/lib/Target/R600/SIDefines.h

enum {
   VOPC = 1 << 13,
 
   MUBUF = 1 << 14,
   MTBUF = 1 << 15,
   SMRD = 1 << 16,
   DS = 1 << 17,
   MIMG = 1 << 18,
   FLAT = 1 << 19,
-  WQM = 1 << 20
+  WQM = 1 << 20,
+  VGPRSpill = 1 << 21
 };
 }
 
 namespace llvm {
 namespace AMDGPU {
   enum OperandType {
     /// Operand with register or 32-bit immediate
     OPERAND_REG_IMM32 = llvm::MCOI::OPERAND_FIRST_TARGET,

llvm/trunk/lib/Target/R600/SIInstrFormats.td

class InstSI <dag outs, dag ins, string asm, list<dag> pattern> :
 
   field bits<1> MUBUF = 0;
   field bits<1> MTBUF = 0;
   field bits<1> SMRD = 0;
   field bits<1> DS = 0;
   field bits<1> MIMG = 0;
   field bits<1> FLAT = 0;
   field bits<1> WQM = 0;
+  field bits<1> VGPRSpill = 0;
 
   // These need to be kept in sync with the enum in SIInstrFlags.
   let TSFlags{0} = VM_CNT;
   let TSFlags{1} = EXP_CNT;
   let TSFlags{2} = LGKM_CNT;
 
   let TSFlags{3} = SALU;
   let TSFlags{4} = VALU;
class InstSI <dag outs, dag ins, string asm, list<dag> pattern> :
 
   let TSFlags{14} = MUBUF;
   let TSFlags{15} = MTBUF;
   let TSFlags{16} = SMRD;
   let TSFlags{17} = DS;
   let TSFlags{18} = MIMG;
   let TSFlags{19} = FLAT;
   let TSFlags{20} = WQM;
+  let TSFlags{21} = VGPRSpill;
 
   // Most instructions require adjustments after selection to satisfy
   // operand requirements.
   let hasPostISelHook = 1;
   let SchedRW = [Write32Bit];
 }
 
 class Enc32 {

llvm/trunk/lib/Target/R600/SIInstrInfo.h

public:
   bool isFLAT(uint16_t Opcode) const {
     return get(Opcode).TSFlags & SIInstrFlags::FLAT;
   }
 
   bool isWQM(uint16_t Opcode) const {
     return get(Opcode).TSFlags & SIInstrFlags::WQM;
   }
 
+  bool isVGPRSpill(uint16_t Opcode) const {
+    return get(Opcode).TSFlags & SIInstrFlags::VGPRSpill;
+  }
+
   bool isInlineConstant(const APInt &Imm) const;
   bool isInlineConstant(const MachineOperand &MO, unsigned OpSize) const;
   bool isLiteralConstant(const MachineOperand &MO, unsigned OpSize) const;
 
   bool isImmOperandLegal(const MachineInstr *MI, unsigned OpNo,
                          const MachineOperand &MO) const;
 
   /// \brief Return true if this 64-bit VALU instruction has a 32-bit encoding.

llvm/trunk/lib/Target/R600/SIInstructions.td

 // this from happening.
 defm SI_SPILL_S32  : SI_SPILL_SGPR <SGPR_32>;
 defm SI_SPILL_S64  : SI_SPILL_SGPR <SReg_64>;
 defm SI_SPILL_S128 : SI_SPILL_SGPR <SReg_128>;
 defm SI_SPILL_S256 : SI_SPILL_SGPR <SReg_256>;
 defm SI_SPILL_S512 : SI_SPILL_SGPR <SReg_512>;
 
 multiclass SI_SPILL_VGPR <RegisterClass vgpr_class> {
-  let UseNamedOperandTable = 1 in {
+  let UseNamedOperandTable = 1, VGPRSpill = 1 in {
     def _SAVE : InstSI <
       (outs),
       (ins vgpr_class:$src, i32imm:$frame_idx, SReg_128:$scratch_rsrc,
            SReg_32:$scratch_offset),
       "", []
     >;
 
     def _RESTORE : InstSI <
       (outs vgpr_class:$dst),
       (ins i32imm:$frame_idx, SReg_128:$scratch_rsrc, SReg_32:$scratch_offset),
       "", []
     >;
-  } // End UseNamedOperandTable = 1
+  } // End UseNamedOperandTable = 1, VGPRSpill = 1
 }
 
 defm SI_SPILL_V32  : SI_SPILL_VGPR <VGPR_32>;
 defm SI_SPILL_V64  : SI_SPILL_VGPR <VReg_64>;
 defm SI_SPILL_V96  : SI_SPILL_VGPR <VReg_96>;
 defm SI_SPILL_V128 : SI_SPILL_VGPR <VReg_128>;
 defm SI_SPILL_V256 : SI_SPILL_VGPR <VReg_256>;
 defm SI_SPILL_V512 : SI_SPILL_VGPR <VReg_512>;

llvm/trunk/lib/Target/R600/SIPrepareScratchRegs.cpp

if (!MBB.isLiveIn(ScratchOffsetReg) &&
       MBB.addLiveIn(ScratchOffsetReg);
     RS.enterBasicBlock(&MBB);
 
     for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end();
          I != E; ++I) {
       MachineInstr &MI = *I;
       RS.forward(I);
       DebugLoc DL = MI.getDebugLoc();
-      switch(MI.getOpcode()) {
-        default: break;
-        case AMDGPU::SI_SPILL_V512_SAVE:
-        case AMDGPU::SI_SPILL_V256_SAVE:
-        case AMDGPU::SI_SPILL_V128_SAVE:
-        case AMDGPU::SI_SPILL_V96_SAVE:
-        case AMDGPU::SI_SPILL_V64_SAVE:
-        case AMDGPU::SI_SPILL_V32_SAVE:
-        case AMDGPU::SI_SPILL_V32_RESTORE:
-        case AMDGPU::SI_SPILL_V64_RESTORE:
-        case AMDGPU::SI_SPILL_V128_RESTORE:
-        case AMDGPU::SI_SPILL_V256_RESTORE:
-        case AMDGPU::SI_SPILL_V512_RESTORE:
+      if (!TII->isVGPRSpill(MI.getOpcode()))
+        continue;
 
-          // Scratch resource
-          unsigned ScratchRsrcReg =
-              RS.scavengeRegister(&AMDGPU::SReg_128RegClass, 0);
 
-          uint64_t Rsrc = AMDGPU::RSRC_DATA_FORMAT | AMDGPU::RSRC_TID_ENABLE |
-                          0xffffffff; // Size
 
-          unsigned Rsrc0 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub0);
-          unsigned Rsrc1 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub1);
-          unsigned Rsrc2 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub2);
-          unsigned Rsrc3 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub3);
 
-          BuildMI(MBB, I, DL, TII->get(AMDGPU::S_MOV_B32), Rsrc0)
-                  .addExternalSymbol("SCRATCH_RSRC_DWORD0")
-                  .addReg(ScratchRsrcReg, RegState::ImplicitDefine);
 
-          BuildMI(MBB, I, DL, TII->get(AMDGPU::S_MOV_B32), Rsrc1)
-                  .addExternalSymbol("SCRATCH_RSRC_DWORD1")
-                  .addReg(ScratchRsrcReg, RegState::ImplicitDefine);
 
-          BuildMI(MBB, I, DL, TII->get(AMDGPU::S_MOV_B32), Rsrc2)
-                  .addImm(Rsrc & 0xffffffff)
-                  .addReg(ScratchRsrcReg, RegState::ImplicitDefine);
 
-          BuildMI(MBB, I, DL, TII->get(AMDGPU::S_MOV_B32), Rsrc3)
-                  .addImm(Rsrc >> 32)
-                  .addReg(ScratchRsrcReg, RegState::ImplicitDefine);
 
-          // Scratch Offset
-          if (ScratchOffsetReg == AMDGPU::NoRegister) {
-            ScratchOffsetReg = RS.scavengeRegister(&AMDGPU::SGPR_32RegClass, 0);
-            BuildMI(MBB, I, DL, TII->get(AMDGPU::SI_SPILL_S32_RESTORE),
-                    ScratchOffsetReg)
-                    .addFrameIndex(ScratchOffsetFI)
-                    .addReg(AMDGPU::SGPR0_SGPR1_SGPR2_SGPR3, RegState::Undef)
-                    .addReg(AMDGPU::SGPR0, RegState::Undef);
-          } else if (!MBB.isLiveIn(ScratchOffsetReg)) {
-            MBB.addLiveIn(ScratchOffsetReg);
-          }
 
-          if (ScratchRsrcReg == AMDGPU::NoRegister ||
-              ScratchOffsetReg == AMDGPU::NoRegister) {
-            LLVMContext &Ctx = MF.getFunction()->getContext();
-            Ctx.emitError("ran out of SGPRs for spilling VGPRs");
-            ScratchRsrcReg = AMDGPU::SGPR0;
-            ScratchOffsetReg = AMDGPU::SGPR0;
-          }
-          MI.getOperand(2).setReg(ScratchRsrcReg);
-          MI.getOperand(2).setIsKill(true);
-          MI.getOperand(2).setIsUndef(false);
-          MI.getOperand(3).setReg(ScratchOffsetReg);
-          MI.getOperand(3).setIsUndef(false);
-          MI.getOperand(3).setIsKill(false);
-          MI.addOperand(MachineOperand::CreateReg(Rsrc0, false, true, true));
-          MI.addOperand(MachineOperand::CreateReg(Rsrc1, false, true, true));
-          MI.addOperand(MachineOperand::CreateReg(Rsrc2, false, true, true));
-          MI.addOperand(MachineOperand::CreateReg(Rsrc3, false, true, true));
-
-          break;
-      }
     }
   }
   return true;
 }