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

[Thomas Symalla via Phabricator via llvm-commits]

tsymalla added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:31
+static cl::opt<bool>
+    DisableSetWavePriority("amdgpu-disable-set-wave-priority",
+                           cl::desc("Disable adjusting wave priority"),
----------------
foad wrote:
> It's a bit more normal to put "Enable" options in AMDGPUTargetMachine, where you can use isPassEnabled.
Probably invert that options so that setting it to true enables the pass


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:130
+
+  if (DisableSetWavePriority)
+    return false;
----------------
Can be moved to the beginning of the function.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:159
+  MachineBasicBlock::iterator I = Entry.begin(), E = Entry.end();
+  while (I != E && !SIInstrInfo::isVALU(*I) && !I->isTerminator())
+    ++I;
----------------
Could you comment that section please?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:163
+
+  // Lower the priorty on edges where control leaves blocks from which
+  // VMEM loads are reachable.
----------------
Typo: priority


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:191
+  for (MachineBasicBlock *MBB : PriorityLoweringBlocks) {
+    MachineBasicBlock::iterator I = MBB->end(), B = MBB->begin();
+    while (I != B) {
----------------
Could be using the reverse iterator


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124246