[PATCH] D134723: [AMDGPU] Set memory bound occupancy based on addressable VGPRs

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:965
       return Occupancy;
-    return (Occupancy < 4) ? Occupancy : 4;
+    unsigned spillThreshold = ST.getTotalNumVGPRs() / ST.getAddressableNumVGPRs();
+    if (spillThreshold < 4)
----------------
I do not understand the logic really. TotalNumVGPR does not tell anything about spilling. I would understand if that is expressed in a maximum occupancy terms, so you return a fraction of maximum occupancy.

Moreover, this limit is really about a number of concurrent potential memory requests so that they do not evict each other or conflict. So if your CU can run more waves you likely want to drop occupancy here and give shader more registers for a memory bound case.

That said, this is not supposed to be a strong limit, if scheduler have to spill it should drop the occupancy below this limit. At least it was written that way at some point. I.e. I do not think this is a right place to fix spilling, you do not know real pressure here anyway. If this limit causes spilling then scheduler itself have to be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134723