[PATCH] D117562: [AMDGPU] Sink immediate VGPR defs if high RP

Wed Jan 26 08:25:58 PST 2022

vangthao added a comment.

In D117562#3272181 <https://reviews.llvm.org/D117562#3272181>, @vpykhtin wrote:

> In D117562#3272067 <https://reviews.llvm.org/D117562#3272067>, @foad wrote:
>
>> Yes it is hard, but what alternative do we have? I think having the compiler make **a** decision about what occupancy it is trying to achieve, is better than not making any decision at all. And the scheduler already makes that decision, it is just not enforced.
>
> I forgot how the scheduler currently works, I thought it just tries not to make the occupancy worse. I don't know how can we make an assumption for occupancy to enforce it. Given this task we could probably find how many sinks we can do and find out the achievable occupancy. This could also include not only loop sinks but any constants holding VGPRs throughout a function.

Right, by the time we hit pre-ra scheduler, occupany may already be decreased by MachineLICM and since the scheduler does not currently have any way to decrease register pressure of live-throughs, it would not have any ability to increase occupancy if it was decreased by MachineLICM hoisting from loops. Enforcing an occupancy target for RA will help in this case. Another issue is I believe the scheduler will allow for more VGPR usage as long as it does not decrease occupancy. This may hide some achievable occupancy if we do not handle this before scheduler or make scheduler aware that it can increase occupancy by sinking. I am not sure if disabling MachineLICM altogether in high RP situations would be the best solution here.

In D117562#3272347 <https://reviews.llvm.org/D117562#3272347>, @arsenm wrote:

> This feels way too specific. Immediate defs aren't really a special case

Immediate defs was the cause of an issue that I saw in a reported regression case so that was the main focus here. I think we can expand this to target all trivially rematerializable instructions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117562