[PATCH] D158368: [AMDGPU][MISCHED] GCNBalancedSchedStrategy.

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1013
 
-  if (GCNSchedStage::shouldRevertScheduling(WavesAfter))
+  if (GCNSchedStage::shouldRevertScheduling(WavesAfter, WavesBefore))
     return true;
----------------
This means we will always prioritize not letting occupancy drop, perhaps at the expensive of throwing away a good ILP schedule.

For this phase specifically, I think we should only do this check if we are using the MaxOccupancy strategy -- or perhaps, implement `computeScheduleMetric` for the MaxOccupancy strategy and use it in place of `GCNSchedStage::shouldRevertScheduling`


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1029
+         (S.computeScheduleMetric(RegionIdx, WavesAfter, WavesBefore) &&
+          !isRegionWithExcessRP());
 }
----------------
Seems to me this phase should just not be run if `!isRegionWithExcessRP()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158368