[AMDGPU][MISCHED] GCNBalancedSchedStrategy. #66634
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-globalisel ChangesThe change implements the scheduling strategy which aims to find a reasonable trade-off between the ILP and occupancy.
|
jrbyrnes
reviewed
Sep 18, 2023
There was a problem hiding this comment.
I have two remaining points to discuss.
We bypass the UnclusteredHighRPStage (in its initGCNRegion) for regions with the following condition:
if ((!DAG.RegionsWithMinOcc[RegionIdx] ||
DAG.MinOccupancy <= InitialOccupancy) &&
!DAG.RegionsWithExcessRP[RegionIdx])
return false;
My question: for the balanced scheduler, should we actually run this phase for RegionsWithMinOcc && !RegionsWithExcessRP?
Should we run ClusteredLowOccStage for any region so long as we have observed an occupancy drop (at least for the experimental version)?
| if (WavesAfter < DAG.MinOccupancy) | ||
| return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| bool OccInitialScheduleStage::shouldRevertScheduling(unsigned WavesAfter) { | ||
| bool OccInitialScheduleStage::shouldRevertScheduling(unsigned WavesAfter, | ||
| unsigned WavesBefore) { | ||
| if (PressureAfter == PressureBefore) |
There was a problem hiding this comment.
For the balanced scheduler, should we really early exit on RP?
| if (PressureAfter == PressureBefore) | ||
| return false; | ||
|
|
||
| if (GCNSchedStage::shouldRevertScheduling(WavesAfter)) |
There was a problem hiding this comment.
We need to keep this in order to preserve MaxOccupancy scheduler's behavior, but it should not be the priority for the balanced scheduler. Maybe the balanced scheduler should just have a different initial stage entirely.
There was a problem hiding this comment.
We need to keep this in order to preserve MaxOccupancy scheduler's behavior, but it should not be the priority for the balanced scheduler. Maybe the balanced scheduler should just have a different initial stage entirely.
If we decide to exactly preserve the MaxOccupancy scheduler's behavior, we'd better write a completely new code for most things. We could try to have a separate set of stages for the balanced scheduler but it still has to communicate with interfaces that are focused on the occupancy. So, I agree that the approach to minimize the existing code changes could be not viable.
| // Revert if may cause spilling. Otherwise rely on the metric computed by the | ||
| // strategy class. Exception: does not make sense to revert the unclustered | ||
| // schedule if we are still in excess RP state as it will not become better. | ||
| return GCNSchedStage::shouldRevertScheduling(WavesAfter, WavesBefore) || |
There was a problem hiding this comment.
Does this preserve the behavior of MaxOccupancy scheduler?
The idea to run this stage for the ExcessRP regions only is correct. This is (I hope temporarily) left aside to avoid vast changes throughout the scheduler. The reason is that the spill-controlling mechanisms heavily rely on the MinOCC set and as soon as we touch this we immediately have to rewrite a lot of things.
The fact we observed an occupancy drop does not make our schedule invalid, right? So, maybe the chance to improve ILP at the cost of worsening the occupancy yet more is not as bad? We need to have more real traces collected from the real program runs to have statistics. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The change implements the scheduling strategy which aims to find a reasonable trade-off between the ILP and occupancy.
For that purpose, it computes the heuristic metric to decide if the current schedule is worth to be kept.
This is an attempt to use the same idea as in the https://reviews.llvm.org/D139710 to replace the shouldRevertScheduling function.
Unlike the https://reviews.llvm.org/D139710 the heuristic is applied to all scheduling stages.