[PATCH] D114652: [AMDGPU] Treat call clobbered return address registers s[30:31] as callee saved

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu marked 2 inline comments as done.
RamNalamothu added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:94-95
       MachineInstrSpan MIS(I, &SaveBlock);
-      const TargetRegisterClass *RC =
-        TRI->getMinimalPhysRegClass(Reg, MVT::i32);
+      const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(
+          Reg, Reg == RI->getReturnAddressReg(MF) ? MVT::i64 : MVT::i32);
 
----------------
RamNalamothu wrote:
> sebastian-ne wrote:
> > Can the MVT be deduced from `getRegSizeInBits(*getPhysRegClass(Reg))`? I think that would look cleaner.
> > Can the MVT be deduced from `getRegSizeInBits(*getPhysRegClass(Reg))`? I think that would look cleaner.
> 
> Will check, thank you.
`getRegSizeInBits(*getPhysRegClass(Reg))` is not useful as `getPhysRegClass(Reg)` gives base register class i.e. SReg_32.


```
/// Return the 'base' register class for this register.
/// e.g. SGPR0 => SReg_32, VGPR => VGPR_32 SGPR0_SGPR1 -> SReg_32, etc.
const TargetRegisterClass *getPhysRegClass(MCRegister Reg) const;
```


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:630-633
+  // The return address register is call clobbered and the CFI needs to track
+  // its location. Hence it is handled specially.
+  if (!MFI->isEntryFunction())
+    reserveRegisterTuples(Reserved, getReturnAddressReg(MF));
----------------
arsenm wrote:
> I don't think it's necessary to reserve this
But otherwise the register allocation thinks it is available for allocation as `s[30:31]` are scratch registers at the moment and its usage with return instruction is hidden with using SI_RETURN through the register allocation and we don't emit Live-ins anymore.

And we will have to reserve it here even after these registers actually get picked from **CSR range** because we want to emit save/restore exclusively so that the unwind info machinery knows it and can generate CFI as needed for return address. That's the reason why all of this got started right.


================
Comment at: llvm/test/CodeGen/AMDGPU/callee-frame-setup.ll:426
     ,~{s30},~{s31}"() #0
-
+  call void @local_empty_func()
   ret void
----------------
RamNalamothu wrote:
> sebastian-ne wrote:
> > I think these tests should continue to work without the need to introduce a call.
> > I think these tests should continue to work without the need to introduce a call.
> 
> Thanks. I will take a re-look at it. I don't recall now why I added the call.
My bad. I understood them wrongly.

Yes, you are right. Removed the calls and fixed the tests accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114652