[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:854
+  if (DAG.RegionsWithMinOcc[RegionIdx]) {
+    DAG.MinOccupancyRegionsMetrics[RegionIdx] = getScheduleMetrics();
+  }
----------------
alex-t wrote:
> rampitec wrote:
> > alex-t wrote:
> > > rampitec wrote:
> > > > You probably do not need to compute it always, just in UnclusteredHighRPStage?
> > > I only need this at the stage preceding the UnclusteredHighRPStage because it is the "MetricBefore" computation. The UnclusteredHighRPStage only runs for the regions which conform to the condition: 
> > > 
> > > ```
> > > bool UnclusteredHighRPStage::initGCNRegion() {
> > >   // Only reschedule regions with the minimum occupancy or regions that may have
> > >   // spilling (excess register pressure).
> > >   if ((!DAG.RegionsWithMinOcc[RegionIdx] ||
> > >        DAG.MinOccupancy <= InitialOccupancy) &&
> > >       !DAG.RegionsWithExcessRP[RegionIdx])
> > >     return false;
> > > 
> > >   return GCNSchedStage::initGCNRegion();
> > > }
> > > ```
> > > What I should have done here, is to avoid this running for the GCNSchedStageID::ClusteredLowOccupancyReschedule stage.
> > > I only need this at the stage preceding the UnclusteredHighRPStage because it is the "MetricBefore" computation. The UnclusteredHighRPStage only runs for the regions which conform to the condition: 
> > > 
> > > ```
> > > bool UnclusteredHighRPStage::initGCNRegion() {
> > >   // Only reschedule regions with the minimum occupancy or regions that may have
> > >   // spilling (excess register pressure).
> > >   if ((!DAG.RegionsWithMinOcc[RegionIdx] ||
> > >        DAG.MinOccupancy <= InitialOccupancy) &&
> > >       !DAG.RegionsWithExcessRP[RegionIdx])
> > >     return false;
> > > 
> > >   return GCNSchedStage::initGCNRegion();
> > > }
> > > ```
> > > What I should have done here, is to avoid this running for the GCNSchedStageID::ClusteredLowOccupancyReschedule stage.
> > 
> > I believe it is better to place it into the `UnclusteredHighRPStage::initGCNSchedStage()`. This will allow to rearrange stages and also skip it if `UnclusteredHighRPStage` itself is skipped.
> I compute, estimate, and store metrics per region and for those with minimal occupancy only. initGCNSchedStage() yet has no regions. The initGCNRegion is also not a proper place as we need a schedule for the given (i.e. previous) stage to be done before we can compute its metrics.
Regions are created by the first stage. By the time `UnclusteredHighRPStage::initGCNSchedStage()` it should be already initialized. The schedule at the initialization is what a previous stage has left.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139710