[PATCH] D124246: [AMDGPU] Adjust wave priority based on VMEM instructions to avoid duty-cycling.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:102-103
+
+bool AMDGPUSetWavePriority::CanLowerPriorityDirectlyInPredecessors(
+    const MachineBasicBlock &MBB, MBBInfoSet &MBBInfos) const {
+  for (const MachineBasicBlock *Pred : MBB.predecessors()) {
----------------
I think this deserves a comment along the lines of: Check that for every predecessor Pred that can reach a VMEM load, none of Pred's successors can reach a VMEM load.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:107-108
+      continue;
+    if (Loops->getLoopFor(Pred))
+      return false;
+    for (const MachineBasicBlock *Succ : Pred->successors()) {
----------------
I don't understand what this is for. If Pred can reach a VMEM load and it is in a loop, then it must surely have at least one successor that can also reach a VMEM load (namely a successor that is on a path around the loop that leads back to Pred), so we will return false anyway on line 111 below.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:136
+  for (MachineBasicBlock &MBB : MF) {
+    for (const MachineInstr &MI : MBB) {
+      if (isVMEMLoad(MI)) {
----------------
Could use:
```
  if (any_of(MBB, [&](const MachineInstr &MI){ return isVMEMLoad(MI); }))
    Worklist.push_back(&MBB);
```
instead of an explicit loop, here and elsewhere - although it is a matter of taste.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124246