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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:30
 #include "llvm/CodeGen/RegisterClassInfo.h"
+#include "llvm/Support/NativeFormatting.h"
 
----------------
Do you still need it?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:848
     DAG.RegionsWithHighRP[RegionIdx] = true;
-
   // Revert scheduling if we have dropped occupancy or there is some other
----------------
Keep this. It is unrelated to the patch.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:955
+#ifndef NDEBUG
+struct comp {
+  bool operator()(std::pair<MachineInstr *, unsigned> A,
----------------
Could you please use a more specific name for the struct?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:972
+  unsigned IPrev = 1;
+  for (auto I : ReadyCycles) {
+    if (I.second > IPrev + 1)
----------------
`auto &I`.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:991
+  unsigned CurrCycle = 0;
+  for (auto SU : InputSchedule) {
+    unsigned ReadyCycle =
----------------
`auto &SU`.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1006
+             << (SumBubbles
+                     ? (SumBubbles * ScheduleMetrics::ScaleFactor) / CurrCycle
+                     : 1)
----------------
Any chance for CurrCycle to be zero?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1040
+             << (SumBubbles
+                     ? (SumBubbles * ScheduleMetrics::ScaleFactor) / CurrCycle
+                     : 1)
----------------
Any chance for CurrCycle to be zero?


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.h:19
 #include "llvm/CodeGen/MachineScheduler.h"
+#include "llvm/Support/NativeFormatting.h"
 
----------------
You do not seem to need it here.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.h:291
+  // computes the given schedule virtual execution time in clocks
+  ScheduleMetrics getScheduleMetrics(const std::vector<SUnit> &InputSchedule);
+  ScheduleMetrics getScheduleMetrics(const GCNScheduleDAGMILive &DAG);
----------------
SUnit is not a small class, how about `SUnit*`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139710