[PATCH] D124195: [AMDGPU] Separate out custom SGPR spills to VGPR during PEI

Fri Oct 28 11:34:52 PDT 2022

cdevadas added a comment.

In D124195#3892023 <https://reviews.llvm.org/D124195#3892023>, @nhaehnle wrote:

> Almost everything surrounding `allocateSGPRSpillToVGPR` and its companion methods is horrible from a coding style perspective. Now, a lot of this horribleness is pre-existing; that said, can you please get it while you're working on this anyway? Couple of things come to mind:
>
> - allocateVGPRForSGPRSpills and allocateVGPRForCustomSGPRSpills are very specialized and interact in uncomfortable ways with ambient state. They absolutely must not be `public`.
> - They are both basically the same method, and their existence only makes sense in the context of the basically identically named `allocateSGPRSpillToVGPR`. Please just merge those methods and inline them, so only `allocateSGPRSpillToVGPR` remains.
> - How about renaming `allocateSGPRSpillToVGPR` to `allocateSGPRSpillToVGPRLanes`? This accounts for the fact that an SGPR spill is neither allocated to an entire VGPR, nor is an SGPR spill necessarily allocated to a single VGPR (it could cross multiple VGPRs depending on how the lane allocation works out)
> - Relying on `WWMSpills.back()` to get the most recently used spill VGPR makes me rather uncomfortable. Since there's persistent tracking of allocated lanes already in the form of `NumVGPR[Custom]SpillLanes`, please just track the currently "open" VGPR explicitly as well.
> - A lot of data is tied to the "custom / non-custom" distinction (which does need a better name). How about defining a struct `SIMachineFunctionInfo::SGPRToVGPRSpills` and moving all the related data in there (FrameIndex to (VGPR,lane) map, next (VGPR, lane) pair)? That will make the case distinction flow more nicely.

Ideally, D124195 <https://reviews.llvm.org/D124195> and D132436 <https://reviews.llvm.org/D132436> are mostly code-refactor and enabler patches for spilling SGPRs into virtual VGPR lanes and they both should have gone in the final patch D124196 <https://reviews.llvm.org/D124196> that does the spill to virtual VGPRs. I want to use the convention `SGPRSpillToVirtVGPRLanes` (for SILowerSGPRSpills) and `SGPRSpillToPhysVGPRLanes` (for SIFrameLowering) for the two maps that track the spill info. But combining them into a single review would make the patch more complex with too many things in one place. So, I have split them into separate reviews. At this point, the SGPR spills at both places go into physical VGPR lanes and I can’t use the aforementioned names for the maps. The original plan was to have a code clean-up after all these patches landed. Yes, `SIMachineFunctionInfo` is currently in a bad shape. I want to move out the spill related tables and methods and place them into SILowerSGPRSpills and SIFrameLowering passes. Yes, planning to introduce a structure (just like SIMachineFunctionInfo::SGPRToVGPRSpills). I can incorporate all the suggestions you mentioned here in the post-cleanup patch.
At this point, there is a lot of common code for spill handling. But after they become spill to Virtual vs Physical VGPRs, the bookkeeping differs, and we can have a better cleanup.
Hope that would be ok. For now, I will change the term “custom” in this review and can use a better name.
I don’t either like the name “custom”. But couldn’t find a better short name. 
How about `PrologEpilogSGPRSpillToVGPRLanes` instead of `SGPRToVGPRCustomSpills`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124195