[PATCH] D124671: [AMDGPU] Do not raise wave priority beyond a specific number of VALU instructions.

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

nhaehnle added a comment.

I don't quite follow the cross-basic-block logic. Given what I understand of the goal of the heuristic, I expect it to be: "insert a s_setprio 0 before the first long section of dense VALU that can happen after a VMEM load (if any)".

Instead of `MBBInfo::LastVMEMLoad` I can see a `MBBInfo::LastVALUSequenceBreak` and an `MBBInfo::PastVMEMLoad` boolean to indicate whether a VMEM load can have been issued at the end of each basic block.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:127-128
+        break;
+      if (SIInstrInfo::isVMEM(MI) && MI.mayLoad())
+        LastVMEMLoad = &MI;
+      if (SIInstrInfo::isVALU(MI))
----------------
I believe NumVALUInsts should be reset to 0 here. It should probably also be reset at a number of other events, in particular DS instructions.

The reasoning is that we want to lower priority just before running a long dense block of VALU, so that other waves have a better chance of running address calculation VALU.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:30-31
+static cl::opt<unsigned> VALUInstsThreshold(
+    "amdgpu-set-wave-priority-valu-insts-threshold",
+    cl::desc("VALU instruction count threshold for adjusting wave priority"),
+    cl::init(100), cl::Hidden);
----------------
kosarev wrote:
> arsenm wrote:
> > Should we be counting cycles instead of instructions?
> We don't know it yet, I'm afraid. For the couple real use cases that we have for the issue that this pass tries to address counting instructions looks sufficient. This being coupled with that proper cycle counting might be not very trivial, we may be at the risk of over-engineering here.
Could this also be configured via a function attribute? Those are less problematic than command-line options in a driver context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124671