[PATCH] D124671: [AMDGPU] Only raise wave priority if there is a long enough sequence of VALU instructions.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:142-143
+      } else if (SIInstrInfo::isVALU(MI)) {
+        if (AtStart)
+          ++Info.NumVALUInstsAtStart;
+        ++NumVALUInstsAtEnd;
----------------
nhaehnle wrote:
> Why are you only counting if AtStart? The idea was: find places where a VMEM load is followed by a long sequence of dense VALU. Lower priority between that VMEM load and the dense VALU.
Thank you for the offline clarification, I understand this better now.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:159-163
+    unsigned MaxNumVALUInsts =
+        std::max(MaxNumVALUInstsInMiddle, NumVALUInstsAtEnd);
+    Info.MayReachVMEMLoad =
+        SuccsMayReachVMEMLoad ||
+        (Info.LastVMEMLoad && MaxNumVALUInsts >= VALUInstsThreshold);
----------------
If the winner is MaxNumVALUInstsInMiddle, I believe the relevant VMEMLoad should be the VMEMLoad that appeared just before the corresponding sequence of VALU instead of the last one.

I think that instead of Info.LastVMEMLoad, we'd want to have Info.LastVMEMLoadBeforeLongVALU (feel free to think of a better name), which is only set here at the end if the threshold is exceeded. The loop above would keep track of the most recently seen VMEMLoad as well as the one corresponding to the longest VALU sequence in the middle so far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124671