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

[Alexander via Phabricator via llvm-commits]

alex-t added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1006
+             << (SumBubbles
+                     ? (SumBubbles * ScheduleMetrics::ScaleFactor) / CurrCycle
+                     : 1)
----------------
rampitec wrote:
> Any chance for CurrCycle to be zero?
Since we have 
```
CurrCycle = ++ReadyCycle;
```
the only chance is an empty input. Since the input is the DAG.SUnits we'd never get there as it is empty.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.cpp:1040
+             << (SumBubbles
+                     ? (SumBubbles * ScheduleMetrics::ScaleFactor) / CurrCycle
+                     : 1)
----------------
rampitec wrote:
> Any chance for CurrCycle to be zero?
No chance. For same reason as at line 1006


================
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);
----------------
rampitec wrote:
> SUnit is not a small class, how about `SUnit*`?
It leverages from passing the llvm::ScheduleDAG::SUnits by reference.
The llvm::ScheduleDAG class member SUnits already has a type std::vector<llvm::SUnit>
To pass the vector of addresses I would have to have a loop that fills it in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139710