AMDGPU: Fix off-by-one in SIRegisterInfo::eliminateFrameIndex

[nhaehnle]

Summary

The method insertNOPs expected the number of wait states to be passed as
parameter, while eliminateFrameIndex passed the immediate argument for the
S_NOP, leading to an off-by-one error. Rename the method to make the
meaning of its parameter clearer. The number of 4 / 5 wait states (which
is what the method has always _tried_ to do according to the comment) is
correct according to the hardware docs.

I stumbled upon this while trying to track down the cause of
https://bugs.freedesktop.org/show_bug.cgi?id=93264. While clearly needed,
this patch unfortunately does not fix that bug...

[• tstellarAMD]

lib/Target/AMDGPU/SIInstrInfo.h

444–445 ↗

(On Diff #42923)

To me this is still confusing. I would probably go a step further and re-name the function insertWaitStates().

Otherwise, LGTM.

[nhaehnle]

Address review comment

[• tstellarAMD]

LGTM.

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

MachineInstrBuilder buildIndirectRead(MachineBasicBlock *MBB,
                                         unsigned Address,
                                         unsigned OffsetReg) const override;
   void reserveIndirectRegisters(BitVector &Reserved,
                                 const MachineFunction &MF) const;
 
   void LoadM0(MachineInstr *MoveRel, MachineBasicBlock::iterator I,
               unsigned SavReg, unsigned IndexReg) const;
 
-  void insertNOPs(MachineBasicBlock::iterator MI, int Count) const;
+  void insertWaitStates(MachineBasicBlock::iterator MI, int Count) const;
 
   /// \brief Returns the operand named \p Op.  If \p MI does not have an
   /// operand named \c Op, this function returns nullptr.
   LLVM_READONLY
   MachineOperand *getNamedOperand(MachineInstr &MI, unsigned OperandName) const;
 
   LLVM_READONLY
   const MachineOperand *getNamedOperand(const MachineInstr &MI,

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

unsigned SIInstrInfo::calculateLDSSpillAddress(MachineBasicBlock &MBB,
   unsigned LDSOffset = MFI->LDSSize + (FrameOffset * WorkGroupSize);
   BuildMI(MBB, MI, DL, get(AMDGPU::V_ADD_I32_e32), TmpReg)
           .addImm(LDSOffset)
           .addReg(TIDReg);
 
   return TmpReg;
 }
 
-void SIInstrInfo::insertNOPs(MachineBasicBlock::iterator MI,
+void SIInstrInfo::insertWaitStates(MachineBasicBlock::iterator MI,
                                    int Count) const {
   while (Count > 0) {
     int Arg;
     if (Count >= 8)
       Arg = 7;
     else
       Arg = Count - 1;
     Count -= 8;
     BuildMI(*MI->getParent(), MI, MI->getDebugLoc(), get(AMDGPU::S_NOP))

llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp

case AMDGPU::SI_SPILL_S32_RESTORE: {
                 .addReg(Spill.VGPR)
                 .addImm(Spill.Lane)
                 .addReg(MI->getOperand(0).getReg(), RegState::ImplicitDefine);
       }
 
       // TODO: only do this when it is needed
       switch (MF->getSubtarget<AMDGPUSubtarget>().getGeneration()) {
       case AMDGPUSubtarget::SOUTHERN_ISLANDS:
-        // "VALU writes SGPR" -> "SMRD reads that SGPR" needs "S_NOP 3" on SI
-        TII->insertNOPs(MI, 3);
+        // "VALU writes SGPR" -> "SMRD reads that SGPR" needs 4 wait states
+        // ("S_NOP 3") on SI
+        TII->insertWaitStates(MI, 4);
         break;
       case AMDGPUSubtarget::SEA_ISLANDS:
         break;
       default: // VOLCANIC_ISLANDS and later
-        // "VALU writes SGPR -> VMEM reads that SGPR" needs "S_NOP 4" on VI
-        // and later. This also applies to VALUs which write VCC, but we're
-        // unlikely to see VMEM use VCC.
-        TII->insertNOPs(MI, 4);
+        // "VALU writes SGPR -> VMEM reads that SGPR" needs 5 wait states
+        // ("S_NOP 4") on VI and later. This also applies to VALUs which write
+        // VCC, but we're unlikely to see VMEM use VCC.
+        TII->insertWaitStates(MI, 5);
       }
 
       MI->eraseFromParent();
       break;
     }
 
     // VGPR register spill
     case AMDGPU::SI_SPILL_V512_SAVE: