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

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:136
+  for (MachineBasicBlock &MBB : MF) {
+    for (const MachineInstr &MI : MBB) {
+      if (isVMEMLoad(MI)) {
----------------
kosarev wrote:
> arsenm wrote:
> > 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?
> Sure, done.
I understand there is going to be some counting for VALU instructions, but not aware of any plans to have thresholds for VMEMs. I guess others may know better.


================
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
----------------
arsenm wrote:
> Does it really need to run right here? Can you move it earlier to share the loop analysis?
The Jay's finding eliminates the need for the loop analysis altogether, but speaking in general because this pass doesn't cause any major changes, we likely want it as late as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124246