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

[Alexander via Phabricator via llvm-commits]

alex-t marked 2 inline comments as done.
alex-t added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1013
 
-  if (GCNSchedStage::shouldRevertScheduling(WavesAfter))
+  if (GCNSchedStage::shouldRevertScheduling(WavesAfter, WavesBefore))
     return true;
----------------
jrbyrnes wrote:
> 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`
I am pretty sure this check is useless in this particular place and could be removed.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1029
+         (S.computeScheduleMetric(RegionIdx, WavesAfter, WavesBefore) &&
+          !isRegionWithExcessRP());
 }
----------------
jrbyrnes wrote:
> Seems to me this phase should just not be run if `!isRegionWithExcessRP()`
Some regions were excess RP and the UnclasteredHighRP stage was called to try making them better. They might no longer have excess RP in case it succeeded.  Unfortunately, decreasing the RP they lost ILP and they might have worse metrics. Thus they would have been reverted. This check is here to avoid this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158368