[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:854
+  if (DAG.RegionsWithMinOcc[RegionIdx]) {
+    DAG.MinOccupancyRegionsMetrics[RegionIdx] = getScheduleMetrics();
+  }
----------------
rampitec wrote:
> 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.
We don't need metrics for the whole function - just for several regions.
To compute and store the metrics for the regions with min occupancy (for which the UnclusteredHighRPStage will actually be done) we would have to have a loop over all regions in the initGCNSchedStage()
I put it into finalizeGCNRegion because I can easily check if the metrics are needed for the concrete region and compute them if necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139710