[PATCH] D134949: [AMDGPU][SIFrameLowering] Use the right frame register in CSR spills

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added a comment.

> For the downstream case, can we just erase the entry in the map after emitting the spill?

Can do that.



================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1108
+      SB.save();
+      FuncInfo->SpillInfoSetIgnored(FramePtrReg, true);
+      LiveRegs.addReg(SGPRForFPSaveRestoreCopy);
----------------
scott.linder wrote:
> I agree with others that this `Ignored` mode is hard to reason about, although now that I think I understand it I'm not certain I have a suggestion to improve it.
> 
> One thought I had was to try to make the case it is needed for "just work" by changing the order SGPR->SGPR spills occur relative to the FP setup. Is there a reason we can't just emit all of the `COPY_TO_SCRATCH_SGPR` spills before we set up FP? In that version the only special case remaining is for managing `FramePtrRegScratchCopy`, and I don't see any way to simplify that further.
> 
> The whole thing would look like:
> 
> ```
> <SGPR->SGPR spills>
> if (<FP is spilled, and the kind of that spill is not COPY_TO_SCRATCH_SGPR>) {
>   <Do the FramePtrRegScratchCopy scavenging>
> }
> <FP and SP setup>
> <All remaining (i.e. non SGPR->SGPR) spills, respecting FramePtrRegScratchCopy>
> ```
> 
> For the downstream case, can we just erase the entry in the map after emitting the spill?
I will remove the Ignored flag entirely and can handle it differently.

We should be able to use SP relative offset to spill CSRs before adjusting the SP at function entry. It currently needs some effort in the AMDGPU frame-lowering implementation to adjust the offsets assigned for the CSR objects before inserting the spill code for various cases, especially when stack realignment is needed. Some targets handle it better (I believe AArch64). 
It's already in the pipeline and should be implemented soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134949