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

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:232
+    Register Reg = TRI->getReturnAddressReg(MF);
+    if (!FuncInfo->isEntryFunction() && MFI.hasCalls()) {
+      const TargetRegisterClass *RC =
----------------
RamNalamothu wrote:
> sebastian-ne wrote:
> > arsenm wrote:
> > > RamNalamothu wrote:
> > > > sebastian-ne wrote:
> > > > > Why is the return address only marked as callee-save if this function has calls? I assume it can’t it be overwritten if there are no calls because it is marked as reserved?
> > > > > 
> > > > > Coming back here after reading the tests, I think s[30:31] should also be marked as callee-save if there are no calls. It shouldn’t cause harm if there are no calls (the register is reserved anyway) and it should fix the tests.
> > > > > Why is the return address only marked as callee-save if this function has calls? I assume it can’t it be overwritten if there are no calls because it is marked as reserved?
> > > > > 
> > > > 
> > > > Yes.
> > > > 
> > > > > Coming back here after reading the tests, I think s[30:31] should also be marked as callee-save if there are no calls. It shouldn’t cause harm if there are no calls (the register is reserved anyway) and it should fix the tests.
> > > > 
> > > > AFAIK that's the eventual goal and there is a discussion to pick the ABI registers for return address from the callee saved range. And these changes should work with no/minimal changes.
> > > > 
> > > > However, I don't think I understand how the tests will get fixed automatically if the return address registers are marked as callee saved because even the SGPR CSR save/restore happens during SGPRSpillLowering which is where I am manually adding s[30:31] to the CSR list.
> > > I don't think this register should be reserved, which avoids the need to do this
> > Why is the return address not callee-save if there are no calls?
> > 
> > Does removing the `&& MFI.hasCalls()` condition have any bad effects?
> No functional issues but its just that it will be unnecessary to save/restore when the registers are not clobbered i.e. when we have no calls.
But if we have no calls and the register is not clobbered, marking the registers as callee-save shouldn’t save and restore them?

I think it doesn’t hurt to remove this condition. On the contrary, I expect that removing the hasCalls condition will fix the case where s30 or s31 are overwritten in inline-assembly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114652