[PATCH] D141728: [AMDGPU] Tune scheduler on GFX10 and GFX11

Mon Jan 16 10:55:04 PST 2023

rampitec added a comment.

In D141728#4055501 <https://reviews.llvm.org/D141728#4055501>, @kerbowa wrote:

> I think this makes sense, but it also makes the concept of an occupancy target a misnomer.
>
> Additionally, I worry that we may be over-prioritizing RP reduction when we consistently see cases where higher RP isn't necessarily leading to better performance.
>
> This situation is basically why the HighRPReschedule stage exists - where we target a higher occupancy to see if occupancy was dropped because the heuristics are not focusing RP reduction soon enough. This HighRP stage will only increase the target occupancy by one and decrease the critical register limit by an additional 10. We could probably be more aggressive with these numbers in cases where we have spilling. For example, setting the HighRPErrorMargin to something very high like 200 also eliminates the spilling in the testcase for the relevant ticket (SWDEV-377300).
>
> That may be a more targeted change, but if your feeling is that this patch will be a more general improvement it looks good to me.

Austin, thanks for the review. I agree this makes concept of occupancy based scheduling somewhat dissolved, although I do not think it completely misrepresents the pass now. This only changes a single threshold while others still use occupancy. This is more of a wording issue to me anyway, but I certainly will try to play with HighRPReschedule, mainly because I am a bit afraid of the amount of gfx10/gfx11 scheduling changes and their potential impact. If that is possible to only tackle high RP issue when we really hit spilling and prioritize ILP otherwise, that would be ideal for sure.

Meanwhile, it would be interesting to see an overall impact of this patch as it is on graphics and gfx10 (possibly gfx11 too). @foad do you mind to help with that? If we see it has an overall improvement we may prefer current approach anyway. That said the issue shall affect wave64 kernels less than wave32.

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

https://reviews.llvm.org/D141728