[AMDGPU] Fix missing waitcnt issue

[piotr]

Summary

Ignore out of order counters when merging brackets. The fact that
there was a pending event in the old state does not guarantee that
the waitcnt was generated, so we still need to conservatively re-process
the block.

The patch fixes a correctness issue where the block was not re-processed
and the waitcnt not inserted in consequence.

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1428

As an experiment, would changing the condition to RegStrictDom && !OldOutOfOrder && MyPending != 0 also fix the bug?

[foad]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1428

As an experiment, would changing the condition to RegStrictDom && !OldOutOfOrder && MyPending != 0 also fix the bug?

Sorry I meant RegStrictDom && !(OldOutOfOrder && MyPending != 0).

[piotr]

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1428

As an experiment, would changing the condition to RegStrictDom && !OldOutOfOrder && MyPending != 0 also fix the bug?

Sorry I meant RegStrictDom && !(OldOutOfOrder && MyPending != 0).

Do you mean this:

if (RegStrictDom && !(OldOutOfOrder && MyPending == 0))

I think that would also fix the bug in the test, but I wasn't convinced that was a complete fix, so I went for a safer and simpler approach.

[foad]

As a bit of background, the OldOutOfOrder test was introduced by @nhaehnle in a large refactoring in D54231. I think it's just a performance optimisation: if the old state had events that completed out of order, then any use of the corresponding registers would have to be preceded by a "waitcnt 0", so there's no need to reprocess that block because the waitcnts are already as strict as they can be. But this goes wrong in the case where the merge introduces a wait on a particular register that had no wait in the old state (so no waitcnt would have been generated for it the last time the block was processed).

[foad]

Did you notice any compile-time degradation from your fix?

[piotr]

In our usual compile-time tests it shows 0.056% degradation on average, worst case 0.9%.

[foad]

In D117544#3250636, @piotr wrote:

In our usual compile-time tests it shows 0.056% degradation on average, worst case 0.9%.

In that case I agree we should go with your fix as it is the simplest.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

1428

Do you mean this:

I'm not sure what I meant; sorry for the noise. Now I think it *might* be possible to salvage the original optimisation by remembering whether any individual register has a wait in the merged state, when it had no wait in the original state. But I don't think it's worth the complexity.

[piotr]

Remove references to "waw" as the issue can also trigger in other scenarios, as Jay pointed out to me (thanks).

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

 bool WaitcntBrackets::merge(const WaitcntBrackets &Other) {
   bool StrictDom = false;
 
   VgprUB = std::max(VgprUB, Other.VgprUB);
   SgprUB = std::max(SgprUB, Other.SgprUB);
 
   for (auto T : inst_counter_types()) {
     // Merge event flags for this counter
-    const bool OldOutOfOrder = counterOutOfOrder(T);
     const unsigned OldEvents = PendingEvents & WaitEventMaskForInst[T];
     const unsigned OtherEvents = Other.PendingEvents & WaitEventMaskForInst[T];
     if (OtherEvents & ~OldEvents)
       StrictDom = true;
     PendingEvents |= OtherEvents;
 
     // Merge scores for this counter
     const unsigned MyPending = ScoreUBs[T] - ScoreLBs[T];
for (auto T : inst_counter_types()) {
     }
 
     if (T == LGKM_CNT) {
       for (int J = 0; J <= SgprUB; J++) {
         RegStrictDom |= mergeScore(M, SgprScores[J], Other.SgprScores[J]);
       }
     }
 
-    if (RegStrictDom && !OldOutOfOrder)
+    if (RegStrictDom)
       StrictDom = true;
   }
 
   return StrictDom;
 }
 
 // Generate s_waitcnt instructions where needed.
 bool SIInsertWaitcnts::insertWaitcntInBlock(MachineFunction &MF,

llvm/test/CodeGen/AMDGPU/waitcnt.mir

--- |
 
   define amdgpu_kernel void @cross_bundle() {
     ret void
   }
 
   define amdgpu_kernel void @subregs16bit() {
     ret void
   }
+
+  define amdgpu_kernel void @waitcnt_backedge() {
+    ret void
+  }
 ...
 ---
 
 # CHECK-LABEL: name: flat_zero_waitcnt
 
 # CHECK-LABEL: bb.0:
 # CHECK: FLAT_LOAD_DWORD
 # CHECK: FLAT_LOAD_DWORDX4
machineFunctionInfo:
   isEntryFunction: true
 body: |
   bb.0:
     liveins: $vgpr0_vgpr1, $vgpr2_vgpr3, $vgpr4
       $vgpr0 = FLAT_LOAD_USHORT killed $vgpr0_vgpr1, 0, 0, implicit $exec, implicit $flat_scr
       $vgpr1 = FLAT_LOAD_USHORT killed $vgpr2_vgpr3, 0, 0, implicit $exec, implicit $flat_scr
       V_NOP_e32 implicit $exec, implicit $vgpr0_lo16, implicit $vgpr1_lo16
 ...
+
+---
+# Waitcnt required before the use of $sgpr10_sgpr11, as the S_LOAD also writes
+# to $sgpr10_sgpr11, and can occur first in the program running order.
+
+# CHECK-LABEL: name: waitcnt_backedge
+# CHECK: S_WAITCNT
+# CHECK: $sgpr10_sgpr11 = S_CSELECT_B64
+# CHECK: $sgpr10_sgpr11 = S_LOAD_DWORDX2_IMM
+
+
+name: waitcnt_backedge
+body: |
+  bb.0:
+    renamable $sgpr4_sgpr5_sgpr6_sgpr7 = S_LOAD_DWORDX4_IMM renamable $sgpr2_sgpr3, 32, 0 :: (load (s128) from `i32 addrspace(4)* undef`, addrspace 4)
+
+  bb.4:
+    renamable $sgpr10_sgpr11 = S_CSELECT_B64 -1, 0, implicit killed $scc
+    renamable $vgpr1 = BUFFER_LOAD_DWORD_OFFEN killed renamable $vgpr5, renamable $sgpr4_sgpr5_sgpr6_sgpr7, 0, 0, 1, 0, 0, implicit $exec :: (dereferenceable load (s32), align 1, addrspace 4)
+    renamable $sgpr10_sgpr11 = S_LOAD_DWORDX2_IMM killed renamable $sgpr0_sgpr1, 0, 0 :: (load (s64) from `i32 addrspace(4)* undef`, align 4, addrspace 4)
+    S_CBRANCH_SCC0 %bb.9, implicit killed $scc
+
+  bb.9:
+    renamable $vgpr1 = V_CNDMASK_B32_e64 0, 0, 0, 1, killed $sgpr14_sgpr15, implicit $exec
+    S_CBRANCH_SCC0 %bb.14, implicit killed $scc
+
+  bb.10:
+    S_BRANCH %bb.4
+
+  bb.14:
+    S_ENDPGM 0
+...