AMDGPU: Don't use offen if it is 0

[arsenm]

Summary

This removes many re-initializations of a base register to 0.

[nhaehnle]

The computation of NewOffset should be shared between the load and store paths.

I guess changing the MachineInstr could also be shared using MachineInstr::RemoveOperand, but the resulting code might not be much cleaner.

The rest looks good to me.

[arsenm]

In D24645#545602, @nhaehnle wrote:

The computation of NewOffset should be shared between the load and store paths.

I guess changing the MachineInstr could also be shared using MachineInstr::RemoveOperand, but the resulting code might not be much cleaner.

The rest looks good to me.

Ma original plan was to just re-use buildScratchLoadStore which is what D24630 was for, but that changes from scavenging a VGPR to an SGPR and I wasn't sure what the consequences of that might be

[arsenm]

Refactor. buildScratchLoadStore is different enough that its should be separate

[arsenm]

ping

[kzhuravl]

LGTM

[arsenm]

r282999

lib/Target/AMDGPU/SIRegisterInfo.h

public:
   ///
   /// \returns Value that meets number of waves per execution unit requirement
   /// if explicitly requested value cannot be converted to integer, violates
   /// subtarget's specifications, or does not meet number of waves per execution
   /// unit requirement.
   unsigned getMaxNumVGPRs(const MachineFunction &MF) const;
 
 private:
-  void buildScratchLoadStore(MachineBasicBlock::iterator MI,
-                             unsigned LoadStoreOp, const MachineOperand *SrcDst,
-                             unsigned ScratchRsrcReg, unsigned ScratchOffset,
-                             int64_t Offset,
-                             RegScavenger *RS) const;
+  void buildSpillLoadStore(MachineBasicBlock::iterator MI,
 };
 
 } // End namespace llvm
 
 #endif

lib/Target/AMDGPU/SIRegisterInfo.cpp

static unsigned getNumSubRegsForSpillOp(unsigned Op) {
   case AMDGPU::SI_SPILL_S32_RESTORE:
   case AMDGPU::SI_SPILL_V32_SAVE:
   case AMDGPU::SI_SPILL_V32_RESTORE:
     return 1;
   default: llvm_unreachable("Invalid spill opcode");
   }
 }
 
-void SIRegisterInfo::buildScratchLoadStore(MachineBasicBlock::iterator MI,
-                                           unsigned LoadStoreOp,
-                                           const MachineOperand *SrcDst,
-                                           unsigned ScratchRsrcReg,
-                                           unsigned ScratchOffset,
-                                           int64_t Offset,
-                                           RegScavenger *RS) const {
-
+static int getOffsetMUBUFStore(unsigned Opc) {
+  switch (Opc) {
+  case AMDGPU::BUFFER_STORE_DWORD_OFFEN:
+    return AMDGPU::BUFFER_STORE_DWORD_OFFSET;
+  case AMDGPU::BUFFER_STORE_BYTE_OFFEN:
+    return AMDGPU::BUFFER_STORE_BYTE_OFFSET;
+  case AMDGPU::BUFFER_STORE_SHORT_OFFEN:
+    return AMDGPU::BUFFER_STORE_SHORT_OFFSET;
+  case AMDGPU::BUFFER_STORE_DWORDX2_OFFEN:
+    return AMDGPU::BUFFER_STORE_DWORDX2_OFFSET;
+  case AMDGPU::BUFFER_STORE_DWORDX4_OFFEN:
+    return AMDGPU::BUFFER_STORE_DWORDX4_OFFSET;
+  default:
+    return -1;
+  }
+}
+
+static int getOffsetMUBUFLoad(unsigned Opc) {
+  switch (Opc) {
+  case AMDGPU::BUFFER_LOAD_DWORD_OFFEN:
+    return AMDGPU::BUFFER_LOAD_DWORD_OFFSET;
+  case AMDGPU::BUFFER_LOAD_UBYTE_OFFEN:
+    return AMDGPU::BUFFER_LOAD_UBYTE_OFFSET;
+  case AMDGPU::BUFFER_LOAD_SBYTE_OFFEN:
+    return AMDGPU::BUFFER_LOAD_SBYTE_OFFSET;
+  case AMDGPU::BUFFER_LOAD_USHORT_OFFEN:
+    return AMDGPU::BUFFER_LOAD_USHORT_OFFSET;
+  case AMDGPU::BUFFER_LOAD_SSHORT_OFFEN:
+    return AMDGPU::BUFFER_LOAD_SSHORT_OFFSET;
+  case AMDGPU::BUFFER_LOAD_DWORDX2_OFFEN:
+    return AMDGPU::BUFFER_LOAD_DWORDX2_OFFSET;
+  case AMDGPU::BUFFER_LOAD_DWORDX4_OFFEN:
+    return AMDGPU::BUFFER_LOAD_DWORDX4_OFFSET;
+  default:
+    return -1;
+  }
+}
+
+// This differs from buildSpillLoadStore by only scavenging a VGPR. It does not
+// need to handle the case where an SGPR may need to be spilled while spilling.
+static bool buildMUBUFOffsetLoadStore(const SIInstrInfo *TII,
+                                      MachineFrameInfo &MFI,
+                                      MachineBasicBlock::iterator MI,
+                                      int Index,
+                                      int64_t Offset) {
+  MachineBasicBlock *MBB = MI->getParent();
+  const DebugLoc &DL = MI->getDebugLoc();
+  bool IsStore = MI->mayStore();
+
+  unsigned Opc = MI->getOpcode();
+  int LoadStoreOp = IsStore ?
+    getOffsetMUBUFStore(Opc) : getOffsetMUBUFLoad(Opc);
+  if (LoadStoreOp == -1)
+    return false;
+
+  unsigned Reg = TII->getNamedOperand(*MI, AMDGPU::OpName::vdata)->getReg();
+
+  BuildMI(*MBB, MI, DL, TII->get(LoadStoreOp))
+    .addReg(Reg, getDefRegState(!IsStore))
+    .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::srsrc))
+    .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::soffset))
+    .addImm(Offset)
+    .addImm(0) // glc
+    .addImm(0) // slc
+    .addImm(0) // tfe
+    .setMemRefs(MI->memoperands_begin(), MI->memoperands_end());
+  return true;
+}
+
+void SIRegisterInfo::buildSpillLoadStore(MachineBasicBlock::iterator MI,
   unsigned Value = SrcDst->getReg();
   bool IsKill = SrcDst->isKill();
   MachineBasicBlock *MBB = MI->getParent();
   MachineFunction *MF = MI->getParent()->getParent();
   const SISubtarget &ST =  MF->getSubtarget<SISubtarget>();
   const SIInstrInfo *TII = ST.getInstrInfo();
 
   DebugLoc DL = MI->getDebugLoc();
void SIRegisterInfo::buildSpillLoadStore(MachineBasicBlock::iterator MI,
   if (RanOutOfSGPRs) {
     // Subtract the offset we added to the ScratchOffset register.
     BuildMI(*MBB, MI, DL, TII->get(AMDGPU::S_SUB_U32), ScratchOffset)
             .addReg(ScratchOffset)
             .addImm(OriginalImmOffset);
   }
 }
 
-static int getOffsetMUBUFStore(unsigned Opc) {
-  switch (Opc) {
-  case AMDGPU::BUFFER_STORE_DWORD_OFFEN:
-    return AMDGPU::BUFFER_STORE_DWORD_OFFSET;
-  case AMDGPU::BUFFER_STORE_BYTE_OFFEN:
-    return AMDGPU::BUFFER_STORE_BYTE_OFFSET;
-  case AMDGPU::BUFFER_STORE_SHORT_OFFEN:
-    return AMDGPU::BUFFER_STORE_SHORT_OFFSET;
-  case AMDGPU::BUFFER_STORE_DWORDX2_OFFEN:
-    return AMDGPU::BUFFER_STORE_DWORDX2_OFFSET;
-  case AMDGPU::BUFFER_STORE_DWORDX4_OFFEN:
-    return AMDGPU::BUFFER_STORE_DWORDX4_OFFSET;
-  default:
-    return -1;
-  }
-}
-
-static int getOffsetMUBUFLoad(unsigned Opc) {
-  switch (Opc) {
-  case AMDGPU::BUFFER_LOAD_DWORD_OFFEN:
-    return AMDGPU::BUFFER_LOAD_DWORD_OFFSET;
-  case AMDGPU::BUFFER_LOAD_UBYTE_OFFEN:
-    return AMDGPU::BUFFER_LOAD_UBYTE_OFFSET;
-  case AMDGPU::BUFFER_LOAD_SBYTE_OFFEN:
-    return AMDGPU::BUFFER_LOAD_SBYTE_OFFSET;
-  case AMDGPU::BUFFER_LOAD_USHORT_OFFEN:
-    return AMDGPU::BUFFER_LOAD_USHORT_OFFSET;
-  case AMDGPU::BUFFER_LOAD_SSHORT_OFFEN:
-    return AMDGPU::BUFFER_LOAD_SSHORT_OFFSET;
-  case AMDGPU::BUFFER_LOAD_DWORDX2_OFFEN:
-    return AMDGPU::BUFFER_LOAD_DWORDX2_OFFSET;
-  case AMDGPU::BUFFER_LOAD_DWORDX4_OFFEN:
-    return AMDGPU::BUFFER_LOAD_DWORDX4_OFFSET;
-  default:
-    return -1;
-  }
-}
-
 void SIRegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator MI,
                                         int SPAdj, unsigned FIOperandNum,
                                         RegScavenger *RS) const {
   MachineFunction *MF = MI->getParent()->getParent();
   MachineRegisterInfo &MRI = MF->getRegInfo();
   MachineBasicBlock *MBB = MI->getParent();
   SIMachineFunctionInfo *MFI = MF->getInfo<SIMachineFunctionInfo>();
   MachineFrameInfo &FrameInfo = MF->getFrameInfo();
switch (MI->getOpcode()) {
 
     // VGPR register spill
     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:
-      buildScratchLoadStore(MI, AMDGPU::BUFFER_STORE_DWORD_OFFSET,
+      buildSpillLoadStore(MI, AMDGPU::BUFFER_STORE_DWORD_OFFSET,
             TII->getNamedOperand(*MI, AMDGPU::OpName::vdata),
             TII->getNamedOperand(*MI, AMDGPU::OpName::srsrc)->getReg(),
             TII->getNamedOperand(*MI, AMDGPU::OpName::soffset)->getReg(),
             FrameInfo.getObjectOffset(Index) +
             TII->getNamedOperand(*MI, AMDGPU::OpName::offset)->getImm(), RS);
       MI->eraseFromParent();
       MFI->addToSpilledVGPRs(getNumSubRegsForSpillOp(MI->getOpcode()));
       break;
     case AMDGPU::SI_SPILL_V32_RESTORE:
     case AMDGPU::SI_SPILL_V64_RESTORE:
     case AMDGPU::SI_SPILL_V96_RESTORE:
     case AMDGPU::SI_SPILL_V128_RESTORE:
     case AMDGPU::SI_SPILL_V256_RESTORE:
     case AMDGPU::SI_SPILL_V512_RESTORE: {
-      buildScratchLoadStore(MI, AMDGPU::BUFFER_LOAD_DWORD_OFFSET,
+      buildSpillLoadStore(MI, AMDGPU::BUFFER_LOAD_DWORD_OFFSET,
             TII->getNamedOperand(*MI, AMDGPU::OpName::vdata),
             TII->getNamedOperand(*MI, AMDGPU::OpName::srsrc)->getReg(),
             TII->getNamedOperand(*MI, AMDGPU::OpName::soffset)->getReg(),
             FrameInfo.getObjectOffset(Index) +
             TII->getNamedOperand(*MI, AMDGPU::OpName::offset)->getImm(), RS);
       MI->eraseFromParent();
       break;
     }
 
     default: {
       if (TII->isMUBUF(*MI)) {
         // Disable offen so we don't need a 0 vgpr base.
-        assert(FIOperandNum ==
+        assert(static_cast<int>(FIOperandNum) ==
                AMDGPU::getNamedOperandIdx(MI->getOpcode(),
                                           AMDGPU::OpName::vaddr));
 
-        const DebugLoc &DL = MI->getDebugLoc();
-        if (MI->mayLoad()) {
-          int OffenToOffset = getOffsetMUBUFLoad(MI->getOpcode());
-          if (OffenToOffset != -1) {
-            int64_t Offset = FrameInfo.getObjectOffset(Index);
-
-            int64_t OldImm = TII->getNamedOperand(*MI, AMDGPU::OpName::offset)->getImm();
-            int64_t NewOffset = OldImm + Offset;
-
-            if (isUInt<12>(NewOffset)) {
-              unsigned DstReg
-                = TII->getNamedOperand(*MI, AMDGPU::OpName::vdata)->getReg();
-              BuildMI(*MBB, MI, DL, TII->get(OffenToOffset), DstReg)
-                .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::srsrc))
-                .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::soffset))
-                .addImm(NewOffset) // offset
-                .addImm(0)  // glc
-                .addImm(0)  // slc
-                .addImm(0); // tfe
-              MI->eraseFromParent();
-              break;
-            }
-          }
-        } else if (MI->mayStore()) {
-          int OffenToOffset = getOffsetMUBUFStore(MI->getOpcode());
-          if (OffenToOffset != -1) {
-            int64_t Offset = FrameInfo.getObjectOffset(Index);
-
-            int64_t OldImm = TII->getNamedOperand(*MI, AMDGPU::OpName::offset)->getImm();
-            int64_t NewOffset = OldImm + Offset;
+        int64_t OldImm
+          = TII->getNamedOperand(*MI, AMDGPU::OpName::offset)->getImm();
 
-            if (isUInt<12>(NewOffset)) {
-              // Disable offen so we don't need a 0 vgpr base
-              BuildMI(*MBB, MI, DL, TII->get(OffenToOffset))
-                .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::vdata))
-                .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::srsrc))
-                .addOperand(*TII->getNamedOperand(*MI, AMDGPU::OpName::soffset))
-                .addImm(NewOffset) // offset
-                .addImm(0)  // glc
-                .addImm(0)  // slc
-                .addImm(0); // tfe
-              MI->eraseFromParent();
-              break;
-            }
-          }
-        }
-      }
+        if (isUInt<12>(NewOffset) &&
+            buildMUBUFOffsetLoadStore(TII, FrameInfo, MI, Index, NewOffset)) {
 
       int64_t Offset = FrameInfo.getObjectOffset(Index);
       FIOp.ChangeToImmediate(Offset);
       if (!TII->isImmOperandLegal(*MI, FIOperandNum, FIOp)) {
         unsigned TmpReg = MRI.createVirtualRegister(&AMDGPU::VGPR_32RegClass);
         BuildMI(*MBB, MI, MI->getDebugLoc(),
                 TII->get(AMDGPU::V_MOV_B32_e32), TmpReg)
                 .addImm(Offset);