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

Mon Nov 14 13:17:14 PST 2022

arsenm added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1104
+      CSB.save();
+      FuncInfo->SpillInfoSetIgnored(FramePtrReg, true);
+      LiveRegs.addReg(SGPRForFPSaveRestoreCopy);
----------------
cdevadas wrote:
> Here is the case why we need `IsIgnored`. When FP itself is spilled and the SGPRSaveKind is COPY_TO_SCRATCH_SGPR, I avoid the temporary copy to preserve the old value of FP. Instead, I spill FP up front (a copy to the selected scratch SGPR) and mark it as ignored while processing all other spills in the usual way. I could have deleted the entry from `CustomSGPRSpills` Map. But the entry is needed for emitting epilogue spill.
> 
> You can see the test case AMDGPU/GlobalISel/call-outgoing-stack-args.ll ( in func_caller_stack) FP is moved to s4 in the prolog and later copied back from s4 to FP in the epilog.
> But in AMDGPU/GlobalISel/localizer.ll (sink_null_insert_pt), FP is temporarily moved to s16 before bumping it and then s16 is spilled into VGPR41 lane 0. Here the temporary copy is needed because the spill to VGPR41 can be inserted only after spilling VGPR41 to preserve its value.
> 
> Another situation where I use the IsIgnored field is in the downstream copy while emitting CFI for the EXEC MASK. There the spill is needed only in the prolog and not in the epilog. So I mark it ignored after emitting the EXEC prolog spill.
This needs to go in a comment on the field (It also might be clearer to just special case these registers cases directly)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134949