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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:1599
         if (ST->hasReadVCCZBug() &&
-            ScoreBrackets.getScoreLB(LGKM_CNT) <
-                ScoreBrackets.getScoreUB(LGKM_CNT) &&
+            ScoreBrackets.getScoreRange(LGKM_CNT) &&
             ScoreBrackets.hasPendingEvent(SMEM_ACCESS)) {
----------------
I would have thought that hasPendingEvent(SMEM_ACCESS) implies hasPendingEvent(LGKM_CNT) - doesn't it?


================
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;
----------------
scott.linder wrote:
> 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.
I don't know why EXP_CNT is handled specially, but I agree your version of the code is simpler and easier to understand.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:1204
     if (ScoreBrackets.getScoreLB(LGKM_CNT) <
             ScoreBrackets.getScoreUB(LGKM_CNT) &&
         ScoreBrackets.hasPendingEvent(SMEM_ACCESS)) {
----------------
foad wrote:
> Can use getScoreRange here.
You're just testing getScoreRange != 0 here, so you could use the new hasPendingEvent overload (I'm not really sure why we have both and which you should prefer)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136677