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

Thu Dec 2 05:37:07 PST 2021

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 =
----------------
sebastian-ne wrote:
> 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.
I tested it and I think marking s[30:31] as callee-save register instead of explicitly saving them here is better.

1. We don’t need `hasCall` to check if s[30:31] may be overwritten.
2. We shouldn’t need to mark the register as reserved (didn’t test this).
3. It “just works”, even if there is inline assembly overwriting s30 or s31
   (and there are no unnecessary saves/restores if it isn’t overwritten).

The last point also means you don’t need to add work-arounds to tests by introducing artificial calls or changing the registers in inline assembly, because it will keep working as is.

See this sketched commit: https://github.com/Flakebi/amdvlk-llvm-project/commit/240040601e0f8849acb85141142408b3e6a32ff9
E.g. it fixed the test here: https://github.com/Flakebi/amdvlk-llvm-project/commit/240040601e0f8849acb85141142408b3e6a32ff9#diff-25d99c15c21155a13557b29a1c0edfa792c19040f02a0d7cd676865b46275713L75-R88

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114652