[AMDGPU] Avoid flushing the vmcnt counter in loop preheaders if not necessary

[bsaleil]

Summary

One of the conditions to flush the vmcnt counter in loop preheaders is: The loop contains a use of a vgpr that is defined out of the loop.
The code currently checks if a waitcnt is needed by looking at the score of that vgpr in the score brackets. This is not enough and may cause the generation of an unnecessary vmcnt flush. This patch fixed that case.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1733

Maybe use .readsReg() here? See comments about undef in the test case. (Note that readsReg is not mutually exclusive with isDef, since a subreg def operand both reads and writes the undeerlying super reg. So you might also need to remove the "else" on line 1752.)

1742

Maybe you just need: if (Brackets.getRegScore(RegNo, VM_CNT) > Brackets.getScoreLB(VM_CNT))?

llvm/test/CodeGen/AMDGPU/waitcnt-vmcnt-loop.mir

570

It is weird to have undef on all the use operands in this test cause that means the instruction doesn't care about the value it reads - so really no waitcnt is required before any of those reads.

[bsaleil]

Addressed review comments, added test cases with and without undef operands.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1733

Sorry but I think I was wrong to suggest readsReg. A subreg write operand is "readsReg" because it "reads" the other subregs of the superreg, but for the purposes of waitcnt insertion we should not consider it as a read operand. So I think you don't need any change on line 1729 after all.

[bsaleil]

@foad sorry for the delay. I removed the .readsReg() code and cleaned a bit the test case.

[arsenm]

llvm/test/CodeGen/AMDGPU/waitcnt-vmcnt-loop.mir

547

Should drop the operand check from the -NOTs

[foad]

LGTM, thanks!

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

for (MachineInstr &MI : *MBB) {
           HasVMemStore = true;
       }
       for (unsigned I = 0; I < MI.getNumOperands(); I++) {
         MachineOperand &Op = MI.getOperand(I);
         if (!Op.isReg() || !TRI->isVectorRegister(*MRI, Op.getReg()))
           continue;
         RegInterval Interval = Brackets.getRegInterval(&MI, TII, MRI, TRI, I);
         // Vgpr use
         if (Op.isUse()) {
           for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
             // If we find a register that is loaded inside the loop, 1. and 2.
             // are invalidated and we can exit.
             if (VgprDef.contains(RegNo))
               return false;
             VgprUse.insert(RegNo);
             // If at least one of Op's registers is in the score brackets, the
             // value is likely loaded outside of the loop.
-            if (Brackets.getRegScore(RegNo, VM_CNT) > 0) {
+            unsigned Score = Brackets.getRegScore(RegNo, VM_CNT);
+            AMDGPU::Waitcnt Wait;
+            Brackets.determineWait(VM_CNT, Score, Wait);
+            if (Wait.hasWaitVmCnt()) {
               UsesVgprLoadedOutside = true;
               break;
             }
           }
         }
         // VMem load vgpr def
         else if (SIInstrInfo::isVMEM(MI) && MI.mayLoad() && Op.isDef())
           for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

struct Waitcnt {
   bool hasWaitExceptVsCnt() const {
     return VmCnt != ~0u || ExpCnt != ~0u || LgkmCnt != ~0u;
   }
 
   bool hasWaitVsCnt() const {
     return VsCnt != ~0u;
   }
 
+  bool hasWaitVmCnt() const {
+    return VmCnt != ~0u;
+  }
+
   bool dominates(const Waitcnt &Other) const {
     return VmCnt <= Other.VmCnt && ExpCnt <= Other.ExpCnt &&
            LgkmCnt <= Other.LgkmCnt && VsCnt <= Other.VsCnt;
   }
 
   Waitcnt combined(const Waitcnt &Other) const {
     return Waitcnt(std::min(VmCnt, Other.VmCnt), std::min(ExpCnt, Other.ExpCnt),
                    std::min(LgkmCnt, Other.LgkmCnt),

llvm/test/CodeGen/AMDGPU/waitcnt-vmcnt-loop.mir

bb.1:
     S_CMP_LG_U32 killed $sgpr3, $sgpr4, implicit-def $scc
     S_CBRANCH_SCC1 %bb.1, implicit killed $scc
     S_BRANCH %bb.2
 
   bb.2:
     S_ENDPGM 0
 
 ...
+---
+
+# This test case checks that we flush the vmcnt counter only if necessary
+# (i.e. if a waitcnt is needed for the vgpr use we find in the loop)
+
+# GFX10-LABEL: waitcnt_vm_necessary
+# GFX10-LABEL: bb.0:
+# GFX10: S_WAITCNT 16240
+# GFX10: renamable $vgpr4
+# GFX10-NOT: S_WAITCNT 16240
+# GFX10-LABEL: bb.1:
+# GFX10-NOT: S_WAITCNT 16240
+
+# GFX9-LABEL: waitcnt_vm_necessary
+# GFX9-LABEL: bb.0:
+# GFX9: S_WAITCNT 3952
+# GFX9: renamable $vgpr4
+# GFX9-NOT: S_WAITCNT 3952
+# GFX9-LABEL: bb.1:
+# GFX9-NOT: S_WAITCNT 3952
+
+name:            waitcnt_vm_necessary
+body:             |
+  bb.0:
+    successors: %bb.1(0x80000000)
+
+    renamable $vgpr0_vgpr1_vgpr2_vgpr3 = GLOBAL_LOAD_DWORDX4 killed renamable $vgpr0_vgpr1, 0, 0, implicit $exec
+    renamable $vgpr4 = BUFFER_LOAD_DWORD_OFFEN undef renamable $vgpr0, undef renamable $sgpr0_sgpr1_sgpr2_sgpr3, 0, 0, 0, 0, 0, implicit $exec
+
+  bb.1:
+    successors: %bb.1(0x40000000)
+
+    renamable $vgpr5 = BUFFER_LOAD_DWORD_OFFEN undef renamable $vgpr0, renamable $sgpr4_sgpr5_sgpr6_sgpr7, 0, 0, 0, 0, 0, implicit $exec
+    S_CBRANCH_SCC1 %bb.1, implicit killed $scc
+    S_ENDPGM 0
+
+...