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

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1082-1083
+
+  if (TRI.hasStackRealignment(MF))
     HasFP = true;
+
----------------
arsenm wrote:
> Don't understand why this is special cased when you also call hasFP below
When FP is used, the spill instructions are going to use FP as the frame register. In that case, the spill instructions should be emitted only after FP is assigned the new value. Otherwise, the spill instructions use SP as before and they are all inserted before SP is bumped to the new value.
I just moved out `TRI.hasStackRealignment(MF)` from the condition below, updated `HasFP` earlier, and then used it to see whether FP is not going to be used in the function along with `!hasFP(MF)` check.



================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1104
+      CSB.save();
+      FuncInfo->SpillInfoSetIgnored(FramePtrReg, true);
+      LiveRegs.addReg(SGPRForFPSaveRestoreCopy);
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:368
   };
+  bool Ignore = false;
 
----------------
arsenm wrote:
> arsenm wrote:
> > I don't understand what this is supposed to mean
> I still don’t understand what ignored really man’s or why it would occur
This is needed to handle the case when FP itself is spilled while FP is used as the frame register in the spill instructions (buffer_load/store).
Better explained with a test case reference in the other comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134949