[PATCH] D132436: [AMDGPU][SIFrameLowering] Unify custom SGPR spill saves and restores (NFC)

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:269
+
+  void copyToScratchSGPR(Register DstReg) const {
+    const TargetRegisterClass *RC = TRI.getPhysRegClass(DstReg);
----------------
cdevadas wrote:
> sebastian-ne wrote:
> > cdevadas wrote:
> > > sebastian-ne wrote:
> > > > Why does this code split the copies into SubRegs and copyFromScratchSGPR doesn’t?
> > > For the purpose of emitting CFI directives for each copy. There is no CFI emitted in the epilogue.
> > > There is more coming in the downstream version of this patch.
> > Hm, does that mean CFI cannot represent register pairs?
> > If so, can we emit a single copy instruction for the super register and multiple CFI instructions for the subregisters?
> Sure, we will get the benefit for 64-bit copies as we have the 'S_MOV_B64' instruction. But for the CFI emission, we need composite expressions for register pairs. @scott.linder to comment about its impact.
> For now, I will change the code to emit a single COPY instruction for the register pair while retaining separate CFI instructions for equivalent SubRegs.
I agree with using the most efficient representation of the copy (sounds like a 64-bit MOV is best) and generating whatever CFI we need to describe it.

In the case of the 64-bit program address space PC and the wave64 EXEC this will be a pair of SGPRs, described via a composite expression in the CFI, like how CD mentions. For every other case we can generate independent CFI instructions to separately describe the SubRegs which correspond to DWARF registers.

For example, we don't have a DWARF register number for the pair SGPR40_SGPR41, whereas we do for SGPR40 and SGPR41 separately, so we can still emit something along the lines of:

$sgpr40_sgpr41 = S_MOV_B64 $sgpr50_sgpr51
CFI_INSTRUCTION register sgpr50, sgpr40
CFI_INSTRUCTION register sgpr51, sgpr41


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132436