[PATCH] D139710: [AMDGPU] MachineScheduler: schedule execution metric added for the UnclusteredHighRPStage

[Alexander via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:854
+  if (DAG.RegionsWithMinOcc[RegionIdx] &&
+      StageID == GCNSchedStageID::OccInitialSchedule) {
+    DAG.MinOccupancyRegionsMetrics[RegionIdx] = getScheduleMetrics();
----------------
alex-t wrote:
> kerbowa wrote:
> > It might be better in UnclusteredHighRPStage::initGCNRegion(), but I guess the problem is that you need the DAG to be built already? Maybe we even need a virtual getSchedulerMetrics(), since I can imagine this being used for more then the UnclusteredHighRPStage eventually, and now it only does something in the initOccStage. If it stays in the initOccStage, can you add an assert that the next stage is UnclusteredHighRP?
> Your guess is quite correct. My first intention was exactly UnclusteredHighRPStage::initGCNRegion(). Then I found that unfortunately, I have no DAG yet :)
What I really need is to get a metric for the schedule before UnclusteredHighRPStage and after it. The only place I need it is UnclusteredHighRPStage::shouldRevertScheduling
where I already have SUnits which reflects the order before and BB which reflects the current order.  So, making the getScheduleMetrics accept a vector of the SUnit looks like a perfect solution. I don't have to care about any stages except the UnclusteredHighRPStage.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:948
+  unsigned CurrCycle = 0;
+  for (auto &MI : DAG) {
+#ifndef NDEBUG
----------------
kerbowa wrote:
> Can't this just iterate over SUnits?
It now does as I pass the SUs vector to the getScheduleMetrics


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139710