[PATCH] D136677: [AMDGPU] Small cleanups in wait counter code

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Thank you for the cleanup! I left some suggestions, but I am also OK with everything as-is



================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:251
 
   bool hasPending() const { return PendingEvents != 0; }
   bool hasPendingEvent(WaitEventType E) const {
----------------
If you did decide to add the `hasPendingEvent` overload I would suggest renaming this as well. All of these have the same semantics, just with different "filters", so I think they make sense as an overload set.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:307-310
     if (T == EXP_CNT) {
-      unsigned UB = ScoreUBs[T] - getWaitCountMax(EXP_CNT);
-      if (ScoreLBs[T] < UB && UB < ScoreUBs[T])
-        ScoreLBs[T] = UB;
+      unsigned UB = ScoreUBs[EXP_CNT] - getWaitCountMax(EXP_CNT);
+      if (ScoreLBs[EXP_CNT] < UB && UB < ScoreUBs[EXP_CNT])
+        ScoreLBs[EXP_CNT] = UB;
----------------
Some more thoughts while reviewing (although not applicable to your changes directly):

  - Why is `EXP_CNT` special here?
  - Is there a particular reason this relies on unsigned integer underflow? Does this map better onto the conceptual model of the scores/counts?

For either/both can anyone devise a comment to clarify? I suggested what I believe to be an equivalent version which I find easier to understand, but I am likely missing something.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:698-715
 
     switch (T) {
     case VM_CNT:
-      OS << "    VM_CNT(" << UB - LB << "): ";
+      OS << "    VM_CNT(" << SR << "): ";
       break;
     case LGKM_CNT:
+      OS << "    LGKM_CNT(" << SR << "): ";
----------------
Nit, but I'd also factor out the remaining common stuff


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:1223
   if (FlushVmCnt) {
-    unsigned UB = ScoreBrackets.getScoreUB(VM_CNT);
-    unsigned LB = ScoreBrackets.getScoreLB(VM_CNT);
-    if (UB - LB != 0)
+    if (ScoreBrackets.getScoreRange(VM_CNT) != 0)
       Wait.VmCnt = 0;
----------------
I would suggest also adding a `hasPendingEvent(InstCounterType T) const` overload to use here, i.e.

```
  bool hasPendingEvent(InstCounterType T) const {
    bool HasPending = PendingEvents & WaitEventMaskForInst[T];
    assert(HasPending == getScoreUB(T) > getScoreLB(T));
    return HasPending;
  }
```


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:1238
 
-  unsigned UB = ScoreBrackets.getScoreUB(VM_CNT);
-  unsigned LB = ScoreBrackets.getScoreLB(VM_CNT);
-  if (UB - LB == 0)
+  if (ScoreBrackets.getScoreRange(VM_CNT) == 0)
     return false;
----------------



================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:1594
         if (ST->hasReadVCCZBug() &&
-            ScoreBrackets.getScoreLB(LGKM_CNT) <
-                ScoreBrackets.getScoreUB(LGKM_CNT) &&
+            ScoreBrackets.getScoreRange(LGKM_CNT) != 0 &&
             ScoreBrackets.hasPendingEvent(SMEM_ACCESS)) {
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136677/new/

https://reviews.llvm.org/D136677