[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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139710