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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:136
+  for (MachineBasicBlock &MBB : MF) {
+    for (const MachineInstr &MI : MBB) {
+      if (isVMEMLoad(MI)) {
----------------
foad wrote:
> 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.
Is it really worthwhile to do this for all blocks? Should there be some kind of memory instruction count threshold?


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


================
Comment at: llvm/test/CodeGen/AMDGPU/llc-pipeline.ll:386
 ; GCN-O1-NEXT:        SI Final Branch Preparation
+; GCN-O1-NEXT:        Machine Natural Loop Construction
+; GCN-O1-NEXT:        Set wave priority
----------------
Does it really need to run right here? Can you move it earlier to share the loop analysis?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124246