[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:
> > 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.
> Sorry, I didn't get your point well.
> 
> Are you saying we **should** save/restore s[30:31] even when we don't have calls i.e. even when s[30:31] is not modified in the current function? Does that help in any way, other than the inline asm scenario?
> 
> For the inline asm scenario, as I already mentioned. I heard that we currently don't have any policy and the ABI registers cannot be clobbered like any other scratch register right?
> And even if we want to handle that scenario, I guess a blanket removal of `hasCalls()` condition is not the best/optimal IMO because we will be emitting unnecessary save/restore if the code inside inline asm does not actually modify s[30:31].
Ups, ignore my previous comment. I should have read the code again before making that comment.

What I thought this code would do, is add s[30:31] to the callee-save register list if this condition is satisfied. I did not see that it is already spilling it, based on the condition.

Let me try what happens if s[30:31] is (unconditionally) added to the CSR list and the code here is removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114652