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

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
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);
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:135
+    for (MachineInstr &MI : MBB) {
+      if (NumVALUInsts >= VALUInstsThreshold)
+        break;
----------------
tsymalla wrote:
> You could early opt-out when `VALUInstsThreshold == 0` (at the beginning), is that correct?
We could, but I'm not sure I know how that might be useful in practice.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:140
+      if (SIInstrInfo::isVALU(MI))
+        ++NumVALUInsts;
+    }
----------------
arsenm wrote:
> You're counting VALU instructions here and above?
Yes, that's not good. Combined the two new loops into one and simplified related code. Thanks.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSetWavePriority.cpp:198-199
+    if (MachineInstr *LastVMEMLoad = MBBInfos[MBB].LastVMEMLoad) {
+      MBB->insertAfter(MachineBasicBlock::instr_iterator(LastVMEMLoad),
+                       Setprio);
+      continue;
----------------
arsenm wrote:
> Why not construct directly at the insert point?
Well, the instruction we create here is not just an auxiliary value that we coincidentally happen to use in both the cases, if that's what you mean. That instruction must be spent in all cases and we do want it be the same instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124671